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

Fix Inserter: Navigate between blocks with left/right #3864

Merged
merged 13 commits into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@Soean
Member

Soean commented Dec 8, 2017

Description

To navigate in NavigableContainer you can now use orientation="both" and use the left/up keys for the previous item and right/down keys for the next item.

Fixes #3850

Soean added some commits Dec 8, 2017

@Soean Soean changed the title from Inserter: Navigate between blocks with left/right to Fix Inserter: Navigate between blocks with left/right Dec 8, 2017

@@ -124,18 +124,10 @@ export class NavigableMenu extends Component {
const eventToOffset = ( evt ) => {
const { keyCode } = evt;
const isVertical = orientation === 'vertical';

This comment has been minimized.

@youknowriad

youknowriad Dec 8, 2017

Contributor

I guess this means the "orientation" prop is useless now and we should remove all its references (docs, code...).
I'm not against this personally, we just have to make sure it's something we want for all NavigableContainer usage

@youknowriad

youknowriad Dec 8, 2017

Contributor

I guess this means the "orientation" prop is useless now and we should remove all its references (docs, code...).
I'm not against this personally, we just have to make sure it's something we want for all NavigableContainer usage

This comment has been minimized.

@youknowriad

youknowriad Dec 8, 2017

Contributor

@jasmussen Are we ok with this change? It makes left and up / down and right work similarily in the inserter and the toolbar and the dropdowns?

@youknowriad

youknowriad Dec 8, 2017

Contributor

@jasmussen Are we ok with this change? It makes left and up / down and right work similarily in the inserter and the toolbar and the dropdowns?

This comment has been minimized.

@Soean

Soean Dec 8, 2017

Member

I removed it in d4df52a, but not sure if it is needed anywhere.

@Soean

Soean Dec 8, 2017

Member

I removed it in d4df52a, but not sure if it is needed anywhere.

This comment has been minimized.

@jasmussen

jasmussen Dec 8, 2017

Contributor

Looking at the behavior in master, I do agree that it's a little weird that pressing down goes right (depending on where you are), whereas pressing right does nothing. As such this seems like a good change. But it also begs the question why down doesn't go down, and right doesn't go right.

But it is the inserter, and there's a rich history in how we got here, so before I say anything definitive I'd love @afercia's thoughts.

@jasmussen

jasmussen Dec 8, 2017

Contributor

Looking at the behavior in master, I do agree that it's a little weird that pressing down goes right (depending on where you are), whereas pressing right does nothing. As such this seems like a good change. But it also begs the question why down doesn't go down, and right doesn't go right.

But it is the inserter, and there's a rich history in how we got here, so before I say anything definitive I'd love @afercia's thoughts.

This comment has been minimized.

@afercia

afercia Dec 8, 2017

Contributor

We've received some feedback during WCUS contributor day while testing the inserter, same confusion about which arrows where expected to work. I think the arrow navigation should allow three "modes", depending on the nature of the UI component:

  • horizontal: LEFT and RIGHT to go previous and next horizontally, e.g. toolbars
  • vertical: UP and DOWN to go previous and next vertically, e.g. single column menus
  • both: "spatial" navigation where UP and LEFT go previous, and DOWN and RIGHT go next

And yes, in the Inserter I'd use "both" 🙂

@afercia

afercia Dec 8, 2017

Contributor

We've received some feedback during WCUS contributor day while testing the inserter, same confusion about which arrows where expected to work. I think the arrow navigation should allow three "modes", depending on the nature of the UI component:

  • horizontal: LEFT and RIGHT to go previous and next horizontally, e.g. toolbars
  • vertical: UP and DOWN to go previous and next vertically, e.g. single column menus
  • both: "spatial" navigation where UP and LEFT go previous, and DOWN and RIGHT go next

And yes, in the Inserter I'd use "both" 🙂

This comment has been minimized.

@youknowriad

youknowriad Dec 8, 2017

Contributor

@Soean Maybe instead of removing the "orientation" entirely, we keep it and add a third option "both" or "vertical-horizontal". That way we could enable this behavior in the inserter but still keep it as is for the BlockToolbar and the Dropdowns. The Dropdown should probably be only "vertical" while the block toolbar should be "horizontal". Thoughts?

@youknowriad

youknowriad Dec 8, 2017

Contributor

@Soean Maybe instead of removing the "orientation" entirely, we keep it and add a third option "both" or "vertical-horizontal". That way we could enable this behavior in the inserter but still keep it as is for the BlockToolbar and the Dropdowns. The Dropdown should probably be only "vertical" while the block toolbar should be "horizontal". Thoughts?

This comment has been minimized.

@Soean

Soean Dec 8, 2017

Member

@youknowriad I had a similar thought. In WAI-ARIA 1.1 is a new default value undefined: The element's orientation is unknown/ambiguous. https://www.w3.org/TR/wai-aria-1.1/#aria-orientation

@Soean

Soean Dec 8, 2017

Member

@youknowriad I had a similar thought. In WAI-ARIA 1.1 is a new default value undefined: The element's orientation is unknown/ambiguous. https://www.w3.org/TR/wai-aria-1.1/#aria-orientation

@youknowriad

Can we have a unit test for this orientation?

Anyway, LGTM 👍

@Soean

This comment has been minimized.

Show comment
Hide comment
@Soean

Soean Dec 8, 2017

Member

@youknowriad Thanks for your feedback.

  • I changed undefined to both, because there are just two directions.
  • I added a test for both.
  • I added both to the docs.
Member

Soean commented Dec 8, 2017

@youknowriad Thanks for your feedback.

  • I changed undefined to both, because there are just two directions.
  • I added a test for both.
  • I added both to the docs.
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 8, 2017

Contributor

Nice, might be better to wait for the release before merging

Contributor

youknowriad commented Dec 8, 2017

Nice, might be better to wait for the release before merging

@youknowriad youknowriad merged commit 69fbc37 into master Dec 13, 2017

3 checks passed

codecov/project 38.35% (+0.08%) compared to 5c886ce
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/navigable-menu-keys branch Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment