-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[hotkeys] Fix sorting for global hotkey groups with labels #2938
[hotkeys] Fix sorting for global hotkey groups with labels #2938
Conversation
Thanks for your interest in palantir/blueprint, @jeroenvisser101! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
2af74d4
to
7712ea8
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.
any way you could add a test to hotkeysTests
for this?
hotkeys.sort((a, b) => { | ||
if (a.global) { | ||
return b.global ? 0 : -1; | ||
if ((a.global && b.global) || (!a.global && !b.global)) { |
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.
how about if (a.global === b.global)
?
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.
Ah! That's a good one
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.
cool, waiting for a push from you.
Hotkey groups with names that are have the global={true} prop should now also be alphabetically sorted. Fixes palantir#2439
7712ea8
to
090dbd4
Compare
@giladgray I added tests, and they do test the sorting, but they only test the appearance order of the labels. I think that should do it, but please let me know if there are better ways of doing that. |
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.
looks good, thanks @jeroenvisser101!
Thanks! |
Hotkey groups with labels that are have the global={true} prop should
now also be alphabetically sorted.
Fixes #2439
Changes proposed in this pull request:
The change will prioritize global hotkeys, but still sort them with other global hotkey groups. If a hotkey group is both global but also has a group name, it will still sort it alphabetically.
Reviewers should focus on:
Sorting and order of global hotkey groups with a group name set.