-
Notifications
You must be signed in to change notification settings - Fork 4
Generalized game sheets to a reusable template #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Created game-sheet directory in UI folder Sheet can be used for other purposes
| }; | ||
|
|
||
| /** | ||
| * Template predicate to determine if game is Japanese style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amrocha Do you think I'm overdoing the function documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah I think for like, self-evident function like this one it would be fine to not have anything, but you have type information there which really helps in vanilla JS so I think it adds something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how useful type information is. I commented some of the functions in detail, and some I left uncommented. Which is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in preparation for other people to take over maintenance of the system? For example, in lines 60-181 it's hard to keep track of the code and see the big idea with the comments in between. If it's for people to sift through and figure out though, this much documentation may be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not going to be around forever so I think it might be important. Do you think it takes too much away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Give me a day or two, I'm gonna download it and try looking at it without as much documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With some editor setups you should be able to collapse comments
|
@TwelveNights @Mooters @amrocha What do you think of the level of documentation here? Is it too much, or just enough for someone who is unfamiliar with the code? |
chllarry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it clarifies the code lot more, and the formatting makes it easier to understand though not exactly easier to read. Still a good change overall for people unfamiliar with code or the way the menu works
Created this to make showing current games and displaying server-side games easier. Just wanted to see what you thought about it.