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

Story Hierarchy - keyboard accessibility #1427

Merged
merged 10 commits into from
Jul 14, 2017
Merged

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Jul 7, 2017

Issue: #151

What I did

I've added accessibility support to the stories tree.
storybook_hierarchy_react-treebeard-keyboard

Next Steps

Support arrow keys to navigate on tree

How to test

run cra-kitchen-sink and press tab, shit + tab and enter (or any other OS alternative =/ ) when focus is on the tree.

@codecov
Copy link

codecov bot commented Jul 7, 2017

Codecov Report

Merging #1427 into release/3.2 will increase coverage by 11.46%.
The diff coverage is 57.14%.

Impacted file tree graph

@@               Coverage Diff                @@
##           release/3.2    #1427       +/-   ##
================================================
+ Coverage         8.89%   20.36%   +11.46%     
================================================
  Files              242      236        -6     
  Lines             5193     5137       -56     
  Branches           660      693       +33     
================================================
+ Hits               462     1046      +584     
+ Misses            4149     3570      -579     
+ Partials           582      521       -61
Impacted Files Coverage Δ
lib/ui/src/modules/ui/components/layout/index.js 78.57% <ø> (+78.57%) ⬆️
...b/ui/src/modules/ui/components/left_panel/index.js 100% <ø> (ø) ⬆️
...i/components/left_panel/stories_tree/tree_style.js 25% <100%> (+25%) ⬆️
...les/ui/components/left_panel/stories_tree/index.js 100% <100%> (ø) ⬆️
...ponents/left_panel/stories_tree/tree_decorators.js 32.85% <48.27%> (-6.17%) ⬇️
...mponents/left_panel/stories_tree/tree_node_type.js 0% <0%> (ø) ⬆️
addons/notes/src/react.js 0% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
addons/knobs/src/react/WrapStory.js 11.11% <0%> (ø) ⬆️
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6efd70...a22cdda. Read the comment docs.

@usulpro
Copy link
Member

usulpro commented Jul 7, 2017

@igor-dv it's cool!

but did you consider to improve the existing Ctrl-Shift-Left/Right shortcuts?

a bit more details about what I mean:

#981 PR introduces one more UI API for jumping to next/prev storyKind

I guess we can use it with hierarchy as well just need to add keyboard shortcuts

advantage: after merging it we can use the same way of navigation both by keyboard and by touch screen on mobile devices

/cc @shilman

@igor-dv
Copy link
Member Author

igor-dv commented Jul 7, 2017

@usulpro - Great feature, would like to see it live !

What I tried to do here is to support the previous stories functionality - basic navigation with tab (actually in the previous navigation pressing on enter did nothing). I think that the core menu and a tab navigation don't collide one with each other, moreover IMO the core menu should support basic accessibility too =)

I will try to improve the existing Ctrl-Shift-Left/Right shortcuts in the next PR =)

What BTW prevents #981 from being merged ?

@usulpro
Copy link
Member

usulpro commented Jul 7, 2017

Good point 👍

support the previous stories functionality - basic navigation with tab

I'm just not sure is it intentional feature or "side effect". I know some people don't like this blue frames #1293 Maybe somebody could clarify?

related to #981 I guess I need to find some time to fix the issues requested by review (and maybe it needs one more review as well)

@igor-dv
Copy link
Member Author

igor-dv commented Jul 7, 2017

Yep, this blue box is not a big art =) But it could be styled appropriately (thinner and not blue for example)

@igor-dv igor-dv requested a review from shilman July 11, 2017 05:51
@igor-dv
Copy link
Member Author

igor-dv commented Jul 11, 2017

Review will be appreciated =)

Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks for adding a11y

}
HeaderDecorator.propTypes = {
style: PropTypes.shape({
title: PropTypes.object,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these things inside the shape should probably all be .isRequired from the looks of how they're used.

base: PropTypes.object,
}).isRequired,
node: PropTypes.shape({
name: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like node.type and .children could be checked as well

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igor-dv this is really great!!! going the extra mile!

@igor-dv igor-dv merged commit 0121a7c into release/3.2 Jul 14, 2017
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.

4 participants