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

Adhere to Mac platform when showing shortcuts on a Mac #9429

Merged
merged 2 commits into from Aug 30, 2018

Conversation

Projects
None yet
4 participants
@jasmussen
Contributor

jasmussen commented Aug 29, 2018

This fixes #9226.

This changes the display of keyboard shortcuts on the Mac to:

  • not have the plus between
  • use unicode symbols for the characters

The larger discussion for why we're doing this is in the ticket referenced, but the gist of it is: this is what MacOS does, and we mean to tap into that same pattern that Mac users will be used to.

Screenshots:

screen shot 2018-08-29 at 14 19 46

screen shot 2018-08-29 at 14 24 45

screen shot 2018-08-29 at 14 24 54

screen shot 2018-08-29 at 14 24 59

screen shot 2018-08-29 at 14 28 49

Adhere to Mac platform when showing shortcuts on a Mac
This fixes #9226.

This changes the display of keyboard shortcuts on the Mac to:

- not have the plus between
- use unicode symbols for the characters

The larger discussion for why we're doing this is in the ticket referenced, but the gist of it is: this is what MacOS does, and we mean to tap into that same pattern that Mac users will be used to.

@jasmussen jasmussen added this to the 3.7 milestone Aug 29, 2018

@jasmussen jasmussen self-assigned this Aug 29, 2018

@jasmussen jasmussen requested review from tofumatt, mtias and talldan Aug 29, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 29, 2018

Contributor

Note that I also changed "bksp" to "backspace" for the backspace keyboard shortcut. But I think I saw PRs fly by that changes this keyboard combo entirely anyway, so perhaps not worth spending too much time worrying about that one.

Contributor

jasmussen commented Aug 29, 2018

Note that I also changed "bksp" to "backspace" for the backspace keyboard shortcut. But I think I saw PRs fly by that changes this keyboard combo entirely anyway, so perhaps not worth spending too much time worrying about that one.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 29, 2018

Contributor

Totally unrelated to this PR, but is there anything at all we can do to not set focus immediately on the close button when a modal opens? I'd prefer nearly any other option 😅

What does Microsoft Windows do?

Contributor

jasmussen commented Aug 29, 2018

Totally unrelated to this PR, but is there anything at all we can do to not set focus immediately on the close button when a modal opens? I'd prefer nearly any other option 😅

What does Microsoft Windows do?

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill
Contributor

ZebulanStanphill commented Aug 29, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 29, 2018

Contributor

That's interesting Zeb, thanks for sharing the link. If we keep the Backspace option, would be worth looking into ambiguating that too. But this is the shortcut that shuts down Gnome, right?

Contributor

jasmussen commented Aug 29, 2018

That's interesting Zeb, thanks for sharing the link. If we keep the Backspace option, would be worth looking into ambiguating that too. But this is the shortcut that shuts down Gnome, right?

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Aug 29, 2018

Contributor

Actually, I use KDE Plasma, not GNOME. But the desktop environment is irrelevant. The shortcut actually shuts down (or in some cases reboots) the X Window System display server, which is currently the most common display server used in Linux distros. Some distros (like Ubuntu) disable the shortcut, but not Arch Linux or Antergos (the Arch-based distro I use), apparently.

Contributor

ZebulanStanphill commented Aug 29, 2018

Actually, I use KDE Plasma, not GNOME. But the desktop environment is irrelevant. The shortcut actually shuts down (or in some cases reboots) the X Window System display server, which is currently the most common display server used in Linux distros. Some distros (like Ubuntu) disable the shortcut, but not Arch Linux or Antergos (the Arch-based distro I use), apparently.

@tofumatt

I say we should add the Backspace shortcut too, but as-is that's cool, go for it 😄

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 30, 2018

Contributor

Noting that the unit tests are failing here, probably just a small breakage.

Contributor

youknowriad commented Aug 30, 2018

Noting that the unit tests are failing here, probably just a small breakage.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

@youknowriad I'm seeing this test failing:

screen shot 2018-08-30 at 10 59 06

But I'm unsure how my code could've affected that.

Contributor

jasmussen commented Aug 30, 2018

@youknowriad I'm seeing this test failing:

screen shot 2018-08-30 at 10 59 06

But I'm unsure how my code could've affected that.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 30, 2018

Contributor

This was is intermittent and is passing on Travis, I was talking about unit test npm run test-unit

Contributor

youknowriad commented Aug 30, 2018

This was is intermittent and is passing on Travis, I was talking about unit test npm run test-unit

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 30, 2018

Contributor

I can take a look a bit after :)

Contributor

youknowriad commented Aug 30, 2018

I can take a look a bit after :)

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

Looking, thanks.

Contributor

jasmussen commented Aug 30, 2018

Looking, thanks.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

I believe I fixed the tests now.

Note that I was a little bit confuse as to whether I fixed the test titles correctly, feel free to have a look.

Contributor

jasmussen commented Aug 30, 2018

I believe I fixed the tests now.

Note that I was a little bit confuse as to whether I fixed the test titles correctly, feel free to have a look.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 30, 2018

Contributor

Looks good to me, let's wait for travis :)

Contributor

youknowriad commented Aug 30, 2018

Looks good to me, let's wait for travis :)

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 30, 2018

Contributor

All green, Riad. Good to merge?

Contributor

jasmussen commented Aug 30, 2018

All green, Riad. Good to merge?

@youknowriad youknowriad merged commit 774ccda into master Aug 30, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.35% but relative coverage increased by +49.42% compared to 6259b23
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/mac-shortcuts branch Aug 30, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 30, 2018

Contributor

Merge? Merge what?

Contributor

youknowriad commented Aug 30, 2018

Merge? Merge what?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment