-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Feature/940 game tree options #942
base: master
Are you sure you want to change the base?
Feature/940 game tree options #942
Conversation
990230f
to
2ab4f3e
Compare
Issue 940 - SabakiHQ#940 When navigating the gametree, it is not obvious how to bookmark or change colors of nodes. To bookmark, you can use hotspots (ctrl+b), and to change color, you can leave comments on the nodes. But neither of these options are available by right clicking a node, so unless you happen to come across this functionality by leaving a comment and happen to notice the impact, there is nothing that tells the user that this functionality exists. This commit rectifies this by adding these options when right clicking a node in the gametree.
2ab4f3e
to
9b4dc6d
Compare
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.
Firstly, let me apologize for the obscene amount of time it's taken me to get to this :( Sorry - but I'm here for it now!
This is a great addition, I agree. The only part of it that I take any issue with is adding the "Toggle Show Comments" button to the right-click menu; it implies that this only opens the comment pane for that particular node, since every other entry on that menu is node-specific. It doesn't seem necessary to have that global option there. After all, if the user has figured out how to enable the game tree from the global View menu then they can probably also figure out how to toggle the comments from the same place too.
So I'm tempted to merge this in but with that menu item removed. Any objections, @RobertChrist?
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 would push this adjustment myself but you haven't enabled maintainer access to your branch; so let me know if you want to make it on your end, or I can just merge your commits on a separate branch 🙂)
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.
(Oh actually I see you do have "Maintainers are allowed to edit this pull request" on, but I guess Github doesn't recognize me as the maintainer because it belongs to the SabakiHQ org and yishn owns that. Oh well, just let me know if you want to make this adjustment or I'll just push a separate branch tomorrow)
Hey, no worries! I'm happy to hear from ya : D You are right that 'Toggle Show Comments' is a little bit of an odd duck here. I thought the same when I added it, which is why I added it as its own menu item, and put it in its own section group. You're the maintainer of the app, if you think it makes sense to remove go for it! I do like the idea of keeping something in this menu, though : ) The reason, is because it took me a long time to learn that adding a comment to a node changes the color, or that one can only see said comments if the comment mode is on. I really liked the idea that by adding all of these options to the right context menu it naturally leads the user to become aware of all the functionality that can edit the color of a game node. But I agree that toggling a global state from a node specific menu is a bit weird. Sitting here now, I'm wondering if maybe the best option would be to remove the 'Toggle Show Comments' option as you suggest, but add a new option to the Annotate menu called 'Add/View Comment'? This would be a node specific option, and could enable the Show Comments mode if it was not already enabled (thereby helping user feature discovery). We could always do that in a future branch as well, of course |
Implements #940