Skip to content
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: add skip to content on panel and story iframe #11066

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Jun 6, 2020

Issue: #4667

What I did

  • The intention of this change is to make navigating through storybook more accessible.

I added a SkipToContent component which is only visible once focused, in three occasions:

1 - As soon as the user lands in Storybook and tabs for the first time, it shows a button to go to the selected story in the navigation.

2 - When the user is navigating in the sidepane, and selects a story, the next tab shows a button to go directly to the iframe of the story. It is only presented if the story is selected.

3 - When selecting the iframe of the story, if the user navigates one tab back, it shows a button to go back to the story in the navigation of the sidepane.

I didn't know exactly where to put the SkipToContent component, so please give me a light 😄 , also any comment to improve the implementation is appreciated.

How to test

Run storybook and keep tabing, here's an example of the flow:
bettera11y

@yannbf yannbf requested review from domyen and shilman June 6, 2020 15:48
@yannbf yannbf self-assigned this Jun 6, 2020
@yannbf yannbf marked this pull request as draft June 6, 2020 18:28
@yannbf yannbf force-pushed the feature/add-skip-to-content branch from 3c8acc9 to a4eb39e Compare June 6, 2020 19:01
- The intention of this change is to make navigating through
storybook more accessible.
@yannbf yannbf force-pushed the feature/add-skip-to-content branch from a4eb39e to 5369ac3 Compare June 6, 2020 19:26
@shilman
Copy link
Member

shilman commented Jun 7, 2020

@yannbf this is cool--great a11y feature!! needs a design eye though. @domyen can you take a quick look?

@ndelangen
Copy link
Member

👏

@domyen
Copy link
Member

domyen commented Jun 9, 2020

Thanks Yann! The idea is pretty cool!

I think it's going to be pretty tough for the user to learn new interactions:

  • Tab, tab, enter to jump to the Canvas.
  • Shift+tab, enter to jump to the Sidebar.

The labels do help clarify the behavior but are cumbersome to integrate into the UI.

I wonder if we can make the UX more straightforward but keep your intent. 🤔

Have you considered binding the left and right arrow keys?
For instance, when an element has been focused via tabbing, bind the left and right arrow keys to jump focus from the sidebar and canvas.

@yannbf
Copy link
Member Author

yannbf commented Jun 10, 2020

Thanks Yann! The idea is pretty cool!

I think it's going to be pretty tough for the user to learn new interactions:

  • Tab, tab, enter to jump to the Canvas.
  • Shift+tab, enter to jump to the Sidebar.

The labels do help clarify the behavior but are cumbersome to integrate into the UI.

I wonder if we can make the UX more straightforward but keep your intent. 🤔

Have you considered binding the left and right arrow keys?
For instance, when an element has been focused via tabbing, bind the left and right arrow keys to jump focus from the sidebar and canvas.

Hey @domyen, thanks for the ideas! So first of all I am far from an expert in accessibility, but the main intention is to help the visually impaired to navigate through Storybook more easily. The "skip to main content" flow is pretty common in applications and tabbing through is what people do when using screen readers, so it's easier for them to read a button which says "jump to the story" rather than figure out somehow that if they press the right key it would go to the story. I like the feature of the keybinding, but once the user is in the story, Storybook should not alter a normal interaction such as pressing the right or left keys, because that will be useful when testing that story.

I'm tagging @ChrisBAshton @angel1199410 @xurxe because I'm sure they have great feedback about this!

@yannbf yannbf changed the title WIP - feat: add skip to content on panel and story iframe feat: add skip to content on panel and story iframe Jul 2, 2020
@yannbf yannbf marked this pull request as ready for review July 2, 2020 07:28
@yannbf
Copy link
Member Author

yannbf commented Jul 2, 2020

Can someone more experienced with a11y give some feedback about this?

@ChrisBAshton
Copy link

Thanks for working on this, @yannbf! A11y-wise it looks like an improvement to me, but I no longer work on Storybook projects and the landscape may have moved on since I raised that issue.

Perhaps @sareh has some thoughts? (👋)

@angel1199410
Copy link

@yannbf You said it perfectly! This is very much a normal pattern in terms of screen reader experiences.

Some helpful WCAG2.0 documentation: Understanding Success Criterion for Bypass Blocks

WebAIM Skip Navigation Link examples

@yannbf
Copy link
Member Author

yannbf commented Jul 28, 2020

Thank you so much for your feedback @ChrisBAshton @angel1199410!

@domyen @shilman given the feedback I'd say this is a good contribution. What can we do to merge this? Let me know and I'll be happy to do some extra effort for this! I think this will break a barrier for visually impaired people.

@ndelangen ndelangen merged commit dcc23b5 into next Jul 28, 2020
@ndelangen ndelangen deleted the feature/add-skip-to-content branch July 28, 2020 09:21
@domyen
Copy link
Member

domyen commented Jul 28, 2020

Hey @ndelangen this requires more UX work. It's not ready to be merged in this state. Can we get to this post 6.0?

Please revert this merge.

@ndelangen
Copy link
Member

I'll revert this @domyen it's indeed a bit too close to 6.0.0 release and needs a bit more polish, you're right

@ndelangen ndelangen restored the feature/add-skip-to-content branch July 29, 2020 08:47
@ndelangen
Copy link
Member

ndelangen commented Jul 29, 2020

@yannbf we'll need to open a new PR back for this, since this PR got merged too hasty (by me). Sorry.

I still think this will be an amazing improvement for 6.1. @domyen told me he'd likely have time to assist with some polish after the 6.0 release.

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

Successfully merging this pull request may close these issues.

6 participants