-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Adds the block hierarchy navigation menu to the header #10545
Conversation
This is working quite well for me. Agreed on needing to fix #9212. We should also update the single column block icon. |
bad4436
to
810b214
Compare
I fixed the selection bug, I added some tests. The technical pieces should be ready. We might want to do some design tweaks now (cc @jasmussen ) before landing. |
Oh I think @jasmussen is AFK this week (sorry for the pings), I'll see what we can do here :). |
14b9418
to
5779576
Compare
I tried tweaking the design as much as I can. Let me know how I should polish it further. and yes, this is ready for a final review. |
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 have some changes to make and then I think this is good to go. I'll approve after I push my changes; if you want to check them out and make sure they're alright please do!
@@ -44,6 +44,10 @@ const globalShortcuts = { | |||
description: __( 'Show or hide the settings sidebar.' ), | |||
ariaLabel: shortcutAriaLabel.primaryShift( ',' ), | |||
}, | |||
{ | |||
keyCombination: access( 'b' ), |
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.
This combination opens the bookmarks menu in Firefox on Windows, so it won't work. I'll find one that does and edit the PR. 😓
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.
Looks like access( 'l' )
will work across browsers and OSes that I've tested (Edge + Firefox/Chrome on Windows/Mac). I'll change it to that for now.
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'll just mention what I posted in slack here. I started compiling a list of browser shortcuts in a spreadsheet a while ago:
https://docs.google.com/spreadsheets/d/1nK1frKawxV7aboWOJbbslbIqBGoLY7gqKvfwqENj2yE/edit#gid=0
I note that we had access( 'l' ) earmarked for Align Left. How about access( 'o' ), as in O for outline?
// Add a paragraph in the first column | ||
await page.keyboard.type( 'First column' ); | ||
|
||
// Navigate to the columns blocks |
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.
We should add a keyboard version of this test; I'll do it, but one that used the shortcut would be smart 😄
The only thing to note here is NVDA reads out "Block Navigation Block Navigation menu" when this is opened. I wonder if the That is a bug but we can tweak it after we merge this (which we should do for UI freeze). Once this is merged either file a follow-up issue and ping me or I'll do it 😄 |
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.
Changed the shortcut to "o" (see: https://wordpress.slack.com/archives/C02QB2JS7/p1539717395000100) and tweaked the tests. This gets the 👍 from me.
Shortcuts: for future reference, worth nothing on the new WAI-ARIA Authoring Practices 1.1 (still not an official WCAG recommendation) there's a pretty good section dedicated to keyboard shortcuts. It's more about what not to do: https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_shortcuts_assigning |
5c37bce
to
45b7686
Compare
I've also made a PR updating the docs with that new keyboard shortcut, would be great if someone could approve: |
One other thing, I noticed this when testing the new menu #10679. I don't think it's a massive issue, but it did cause some weirdness. |
This is a good thing but it should include an 'explode blocks' toggle, if it doesn't already, which makes all the borders visible and adds padding between them. |
@simbalion and @paaljoachim: Interesting ideas about expectations from these features. I actually think they'd go together; an outline of the selected block would be a great addition. And then a mover would make sense because you could see what you're moving. Not sure about the mover but the outline I'd definitely 👍 as an idea. Could you both file new issues for the features you'd like? This PR is merged so it's not the best place to discuss additional features, but new issues would be great. Thanks! |
@tofumatt Moving blocks would work presumably the same as it normally does, the only difference would be the visibility of each block's borders, and the padding between them. Blocks could be quickly and visibly arranged, and then the 'explode' status could be toggled off to return to WYSIWYG editing. It should still be possible to edit blocks as normal in the exploded state. |
closes #9628 and closes #9212
This is still WIP, there are some things we need to polish before landing this:
Doesn't mean we can't start testing and reviewing the PR though.