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
Use "Command" instead of special character ⌘ on web #3353
Conversation
@@ -966,7 +966,8 @@ extension LogicalKeySetExtension on LogicalKeySet { | |||
return sortedKeys.map((key) { | |||
if (_modifiers.contains(key)) { | |||
if (isMacOS && key == LogicalKeyboardKey.meta) { | |||
return '⌘'; | |||
// TODO(issue #3352) Switch back to using ⌘ once supported on web. | |||
return kIsWeb ? 'Command-' : '⌘'; |
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.
will there be a space on the other side of the dash before the hotkey? e.g. Command- P
, or will it be Command-P
? Either way, we should have a consistent number of spaces on either side of this dash. Also, slight preference for cmd
to be more similar to how we write an abbreviated ctrl
. If we use cmd
, I don't think the dash is 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.
No space before the "P", it looks like this:
I chose Command
to be consistent with how we are showing the modifier names here: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/utils.dart#L943-L948, but happy to change to cmd
instead. Wdyt?
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, I thought we were using ctrl
. In that case, lgtm on what you have!
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.
You may need to skip the 'meta mac' test for web. See other examples of this skip: kIsWeb
. If you skip that test add a TODO link to stop skipping that links to the issue you filed
See #3352 for details
Temporary workaround until rendering issue is fixed in Flutter.