Skip to content

Conversation

@TYMichaelChen
Copy link
Contributor

What is this PR for?

Right now the keyboard shortcut modal shows 'alt' but on macs 'alt' maps to 'opt'. Could be confusing to some people who don't see 'alt' on the keyboard on their macs.

There are many way to achieve this mapping. I used this to minimize code change and take advantage of when paragraphs check for appVersion to set keyboard shortcuts.

Open to suggestions on best approach to solve this problem.

What type of PR is it?

Improvement

Todos

Is there a relevant Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-626

How should this be tested?

  1. Emulate browser to be Mac and open keyboard shortcut
  2. Emulate browser to be Windows and open keyboard shortcut

Screenshots (if appropriate)

Macs:
mac

Windows:
others

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

@felixcheung
Copy link
Member

looks good I think - is it possible to use the fancy mac key like ?
Seem like either we have Option or its short form is usually

https://support.apple.com/en-us/HT201236

@TYMichaelChen
Copy link
Contributor Author

@felixcheung good suggestions. However, for consistency (since we don't have symbols for shift, ctrl, enter etc), I think Opt would be better than ⌥. What do you think?

@felixcheung
Copy link
Member

There's in fact symbols for each of shift, ctrl and enter which are fairly commonly used on mac in apps.
But in any case, how about we put Option in full? It's just not common to see "Opt"

@corneadoug
Copy link
Contributor

Maybe we could change:
Control in Note -> Note Shortcuts
Control in Note Editor -> Editor Shortcuts

What do you guys think?

@TYMichaelChen
Copy link
Contributor Author

@felixcheung : yeah. I know. I meant that in Zeppelin currently, we're not using any of the symbols, so to stay consistent with Zeppelin's current shortcuts, we should stay with Option. I updated the code to spell out option.

@corneadoug : Thanks for suggestion. That's a good idea. I updated the title to reflect that change.

@felixcheung
Copy link
Member

looks good, thanks!
merging if no more discussion.

@asfgit asfgit closed this in 5e89e75 Jan 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants