Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(a11y): add default children behavior to behaviors #1282

Merged
merged 16 commits into from
May 6, 2019

Conversation

silviuaavram
Copy link
Collaborator

@silviuaavram silviuaavram commented May 2, 2019

Fixes #1008

It adds child behaviors by default to Menu related behaviors:

  • menuBehavior/submenuBehavior - menuItemBehavior
  • tabListBehavior - tabBehavior
  • toolbarBehavior - toolbarButtonBehavior

@silviuaavram silviuaavram added accessibility All the Accessibility tasks and bugs should be tagged with this. 🚧 WIP labels May 2, 2019
@@ -160,6 +160,7 @@ export interface AccessibilityDefinition {
focusZone?: FocusZoneDefinition
focusTrap?: FocusTrapDefinition
autoFocus?: AutoFocusZoneDefinition
childrenBehavior?: Accessibility
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name it childBehavior as it is only one.

Copy link
Member

Choose a reason for hiding this comment

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

But, why it should be single? We also have MenuDivider that can be iterated in the same scope...

Copy link
Contributor

Choose a reason for hiding this comment

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

I the sense of AccessibilityBehavior type, it can have only one child behavior. For e.g.

MenuBehavior= (props) => {
...
childBehavior: menuItemBehavior
}

Copy link
Member

Choose a reason for hiding this comment

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

It's not true. menuBehavior has multiple siblings: menuItemBehavior, menuDividerBehavior & submenuBehavior. I had an offline conversation with @miroslavstastny and we agreed on the same thing that was proposed originally:

menuBehavior= (props) => {
  // ...
  childBehaviors: {
     item: menuItemBehavior,
     divider: menuDividerBehavior,
  }
}

It allows to override behaviors for all child items and it's aligned with existing structure on attributes and keyHandlers. Please take one more look on this change.

@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #1282 into master will decrease coverage by 0.04%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   72.77%   72.73%   -0.05%     
==========================================
  Files         736      736              
  Lines        5627     5633       +6     
  Branches     1624     1630       +6     
==========================================
+ Hits         4095     4097       +2     
- Misses       1527     1529       +2     
- Partials        5        7       +2
Impacted Files Coverage Δ
...lib/accessibility/Behaviors/Tab/tabListBehavior.ts 100% <ø> (ø) ⬆️
...ib/accessibility/Behaviors/Menu/submenuBehavior.ts 100% <ø> (ø) ⬆️
...accessibility/Behaviors/Toolbar/toolbarBehavior.ts 100% <ø> (ø) ⬆️
...c/lib/accessibility/Behaviors/Menu/menuBehavior.ts 100% <ø> (ø) ⬆️
packages/react/src/components/Menu/Menu.tsx 68.57% <50%> (-7.3%) ⬇️

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 426e733...2c4987b. Read the comment docs.

@@ -180,6 +184,7 @@ class Menu extends AutoControlledComponent<ReactProps<MenuProps>, MenuState> {
variables,
styles: styles.divider,
inSubmenu: submenu,
accessibility: accessibility.childBehaviors.divider,
Copy link
Contributor

Choose a reason for hiding this comment

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

childBehaviors is optional, check its existence before accessing divider

@@ -203,6 +208,7 @@ class Menu extends AutoControlledComponent<ReactProps<MenuProps>, MenuState> {
active,
inSubmenu: submenu,
indicator,
accessibility: accessibility.childBehaviors.item,
Copy link
Contributor

Choose a reason for hiding this comment

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

childBehaviors is optional, check its existence before accessing item

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility All the Accessibility tasks and bugs should be tagged with this. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Accessibility children behavior
4 participants