-
-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
feat(blocks): add keyboard shortcuts for modifiers #18581
Conversation
Size Change: -55.7 kB (-4%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
* Modifier keyboard shortcuts | ||
*/ | ||
const handleKeyboardShortcuts = (event) => { | ||
const isMac = /Mac|iPod|iPhone|iPad/.test(navigator.userAgent); |
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.
The userAgent easily be modified by users (e.g. for privacy reasons). It looks like navigator.platform could be more solid
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.
Do you need to know? Couldn't you just look for ctrl first then cmd? Or do you both think it's too loose. My experience with user agent is it's not as reliable as you want 😓
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.
Navigator platform is deprecated, https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform
If we check ctrl
first and then cmd
, so both options will work on all the devices.
packages/core/admin/admin/src/content-manager/components/BlocksEditor/BlocksInput/index.js
Outdated
Show resolved
Hide resolved
packages/core/admin/admin/src/content-manager/components/BlocksEditor/BlocksInput/index.js
Outdated
Show resolved
Hide resolved
packages/core/admin/admin/src/content-manager/components/BlocksEditor/index.js
Show resolved
Hide resolved
...es/core/admin/admin/src/content-manager/components/BlocksEditor/plugins/withDataInsertion.js
Outdated
Show resolved
Hide resolved
* Modifier keyboard shortcuts | ||
*/ | ||
const handleKeyboardShortcuts = (event) => { | ||
const isCtrlOrCmd = event.metaKey || event.ctrlKey; |
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 think this would make the shortcut on windows "windows key + event.key", instead of "ctrl + event.key". Because on windows the metaKey is the windows key.
If it's confirmed then I would go back to your 1st implementation since navigator.platform is deprecated
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 probably do the inverse of this, but pressing the windows key summons things on the OS level so I wouldn't be worried about it triggering. The issue with using a regex to detect a user agent is that you have to keep that up to date and you're actually only assuming there are two OS' which in fact for a desktop there are many iirc linux comes in many flavours.
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.
Agree about regex, I will confirm with product having both keys working.
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 just confirmed with Yannis, its fine to have both combinations working.
packages/core/admin/admin/src/content-manager/components/BlocksEditor/BlocksInput/index.js
Outdated
Show resolved
Hide resolved
...es/core/admin/admin/src/content-manager/components/BlocksEditor/plugins/withDataInsertion.js
Outdated
Show resolved
Hide resolved
...es/core/admin/admin/src/content-manager/components/BlocksEditor/plugins/withDataInsertion.js
Outdated
Show resolved
Hide resolved
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.
Tested ✅
* feat: added keyboard shortcuts for modifiers * fix: eventKeys are added per modifier, updated url validation with URL() * fix: replaced eventKey with isValidEventKey() in modifier store * refactor: moved insertData to withLinks plugin
What does it do?
Added keyboard shortcuts for modifiers
How to test it?
User should be able to use common shortcuts like:
Ctrl(Cmd) + b: Transform the selection into bold.
Ctrl(Cmd) + i: Transform the selection into italic.
Ctrl(Cmd) + u: Transform the selection into underline.
Ctrl(Cmd) + v: Add a link to the selection.
Ctrl(Cmd) + e: Transform the selection into inline code.
Ctrl(Cmd) + Shift + s: Transform the selection into strikethrough.