-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add new keyboard shortcuts for block settings menu #8279
Changes from 25 commits
ae3cfe3
ffe5104
15450f0
40d5eb9
97acd9e
001c693
2fa5473
0d4dfb2
375bb3a
030eabf
86b4d02
0fbdd4e
625fd30
68b9eb7
2759893
24fff39
98172cb
06b72b5
fc5a691
8a66776
d4f087c
8ea43bf
783821d
c860d5d
aad3406
f59bdfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,11 @@ import { rawShortcut, displayShortcut } from '@wordpress/keycodes'; | |
|
||
export default { | ||
toggleEditorMode: { | ||
value: rawShortcut.secondary( 'm' ), | ||
label: displayShortcut.secondary( 'm' ), | ||
raw: rawShortcut.secondary( 'm' ), | ||
display: displayShortcut.secondary( 'm' ), | ||
}, | ||
toggleSidebar: { | ||
raw: rawShortcut.primaryShift( ',' ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shortcut is opening a new tab in chrome (Chrome support page) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder is this is another keyboard layout issue, as this key combination has no effect on my keyboard (cmd+shift+,). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably, I have a french Keyboard There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like there's a However, on a British keyboard layout, when you press That seems completely inconsistent, and I don't know why that's the case. |
||
display: displayShortcut.primaryShift( ',' ), | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ import IconButton from '../icon-button'; | |
* | ||
* @return {WPElement} More menu item. | ||
*/ | ||
function MenuItem( { children, className, icon, onClick, shortcut, isSelected = false } ) { | ||
function MenuItem( { children, className, icon, onClick, shortcut, isSelected, role = 'menuitem', ...props } ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: I guess if we remove the role explicit prop and just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking was that |
||
className = classnames( 'components-menu-item__button', className, { | ||
'has-icon': icon, | ||
} ); | ||
|
@@ -40,7 +40,9 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected = | |
className={ className } | ||
icon={ icon } | ||
onClick={ onClick } | ||
aria-pressed={ isSelected } | ||
aria-checked={ isSelected } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be always aria-checked or only when the role is "menuitemcheckbox"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep - I'm relying on an undefined value preventing that attribute from being set on the outputted Both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but please see my last comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you say it's better to just use |
||
role={ role } | ||
{ ...props } | ||
> | ||
{ children } | ||
<Shortcut shortcut={ shortcut } /> | ||
|
@@ -52,7 +54,9 @@ function MenuItem( { children, className, icon, onClick, shortcut, isSelected = | |
<Button | ||
className={ className } | ||
onClick={ onClick } | ||
aria-pressed={ isSelected } | ||
aria-checked={ isSelected } | ||
role={ role } | ||
{ ...props } | ||
> | ||
{ children } | ||
<Shortcut shortcut={ shortcut } /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -393,7 +393,7 @@ export class BlockListBlock extends Component { | |
const shouldAppearSelectedParent = ! showSideInserter && hasSelectedInnerBlock && ! isTypingWithinBlock; | ||
// We render block movers and block settings to keep them tabbale even if hidden | ||
const shouldRenderMovers = ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock; | ||
const shouldRenderBlockSettings = ( isSelected || hoverArea === 'right' ) && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock; | ||
const shouldRenderBlockSettings = ( isSelected || hoverArea === 'right' ) && ! isMultiSelecting && ! isPartOfMultiSelection; | ||
const shouldShowBreadcrumb = isHovered && ! isEmptyDefaultBlock; | ||
const shouldShowContextualToolbar = ! showSideInserter && ( ( isSelected && ! isTypingWithinBlock && isValid ) || isFirstMultiSelected ) && ( ! hasFixedToolbar || ! isLargeViewport ); | ||
const shouldShowMobileToolbar = shouldAppearSelected; | ||
|
@@ -499,7 +499,7 @@ export class BlockListBlock extends Component { | |
<BlockSettingsMenu | ||
clientIds={ clientId } | ||
rootClientId={ rootClientId } | ||
isHidden={ ! ( isHovered || isSelected ) || hoverArea !== 'right' } | ||
isHidden={ ! ( isHovered || isSelected ) || hoverArea !== 'right' || isTypingWithinBlock } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤯 |
||
/> | ||
) } | ||
{ shouldShowBreadcrumb && ( | ||
|
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.
What does this do?
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.
Added comments to explain it. The only shortcut I know that clashes is the one for duplicate, which overrides 'bookmark all open tabs' in chrome. Also, with the devtools open it repositions the devtools pane (Google override their own shortcut). It's not ideal to override a browser shortcut, but the options for a shortcut here are very limited.
Cmd+D is add bookmark
Cmd+Opt+D is show/hide dock
Other uses of preventDefault don't prevent any known shortcuts, but the user might be using an obscure browser/ plugin/ some kind of custom shortcut, and generally we don't want two things to happen when a user uses a shortcut.