feat(Menu): Accessibility - move MenuItem onClick handler from li to a (

#61) * feat(Menu): Accessibility - move MenuItem onClick handler from li to a * - fixed conformance test for MenuItem * underlined MenuItem border px -> rem * closes #44
microsoft · Aug 9, 2018 · a38523e · a38523e
1 parent 167bf51
commit a38523e
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 25 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ### Fixes
 
 - Add selection property to child items in ListExampleSelection so that styles and roles are applied properly @jurokapsiar ([#70](https://github.com/stardust-ui/react/pull/70))
+- Menu `onClick` handler moved from `li` to `a` (accessibility) @miroslavstastny ([#61](https://github.com/stardust-ui/react/pull/61))
+
 
 <!--------------------------------[ v0.2.6 ]------------------------------- -->
 ## [v0.2.6](https://github.com/stardust-ui/react/tree/v0.2.6) (2018-08-09)

diff --git a/src/components/Menu/MenuItem.tsx b/src/components/Menu/MenuItem.tsx
@@ -86,17 +86,13 @@ class MenuItem extends UIComponent<any, any> {
     const { children, content } = this.props
 
     return (
-      <ElementType
-        className={classes.root}
-        onClick={this.handleClick}
-        {...accessibility.attributes.root}
-        {...rest}
-      >
+      <ElementType className={classes.root} {...accessibility.attributes.root} {...rest}>
         {childrenExist(children) ? (
           children
         ) : (
           <a
             className={cx('ui-menu__item__anchor', classes.anchor)}
+            onClick={this.handleClick}
             {...accessibility.attributes.anchor}
           >
             {content}

diff --git a/src/themes/teams/components/Menu/menuItemStyles.ts b/src/themes/teams/components/Menu/menuItemStyles.ts
@@ -1,7 +1,7 @@
 import { pxToRem } from '../../../../lib'
 
 const underlinedItem = (color: string) => ({
-  borderBottom: `solid 4px ${color}`,
+  borderBottom: `solid ${pxToRem(4)} ${color}`,
   transition: 'color .1s ease',
 })
 
@@ -37,8 +37,6 @@ export default {
       lineHeight: 1,
       position: 'relative',
       verticalAlign: 'middle',
-      padding: `${pxToRem(14)} ${pxToRem(18)}`,
-      cursor: 'pointer',
       display: 'block',
       ...(shape === 'pills' && {
         ...(vertical ? { margin: `0 0 ${pxToRem(5)} 0` } : { margin: `0 ${pxToRem(8)} 0 0` }),
@@ -65,12 +63,6 @@ export default {
             background: variables.typePrimaryActiveBackgroundColor,
           }),
         }),
-        ...(shape === 'underlined' && {
-          ...underlinedItem(variables.defaultActiveBackgroundColor),
-          ...(type === 'primary' && {
-            ...underlinedItem(variables.typePrimaryActiveBorderColor),
-          }),
-        }),
       },
 
       ...(active && {
@@ -114,8 +106,37 @@ export default {
             }),
           },
         }),
+      }),
+    }
+  },
+
+  anchor: ({ props, variables }) => {
+    const { active, shape, type } = props
+
+    return {
+      color: 'inherit',
+      display: 'block',
+      ...(shape === 'underlined'
+        ? { padding: `0 0 ${pxToRem(8)} 0` }
+        : { padding: `${pxToRem(14)} ${pxToRem(18)}` }),
+      cursor: 'pointer',
+
+      ':hover': {
+        color: 'inherit',
         ...(shape === 'underlined' && {
+          paddingBottom: `${pxToRem(4)}`,
+          ...underlinedItem(variables.defaultActiveBackgroundColor),
+          ...(type === 'primary' && {
+            ...underlinedItem(variables.typePrimaryActiveBorderColor),
+          }),
+        }),
+      },
+
+      ...(active &&
+        shape === 'underlined' && {
           color: variables.defaultColor,
+          paddingBottom: `${pxToRem(4)}`,
+          ':hover': {},
           ...underlinedItem(variables.defaultActiveColor),
           ...(type === 'primary'
             ? {
@@ -126,13 +147,6 @@ export default {
                 fontWeight: '700',
               }),
         }),
-      }),
     }
   },
-  anchor: () => ({
-    color: 'inherit',
-    ':hover': {
-      color: 'inherit',
-    },
-  }),
 }
diff --git a/test/specs/components/Menu/Menu-test.tsx b/test/specs/components/Menu/Menu-test.tsx
@@ -27,7 +27,11 @@ describe('Menu', () => {
       const items = getItems()
       const menuItems = mountWithProvider(<Menu items={items} />).find('MenuItem')
 
-      menuItems.first().simulate('click')
+      menuItems
+        .first()
+        .find('a')
+        .first()
+        .simulate('click')
       expect(items[0].onClick).toHaveBeenCalled()
     })
 
@@ -48,7 +52,11 @@ describe('Menu', () => {
         const wrapper = mountWithProvider(<Menu items={getItems()} />)
         const menuItems = wrapper.find('MenuItem')
 
-        menuItems.at(1).simulate('click')
+        menuItems
+          .at(1)
+          .find('a')
+          .first()
+          .simulate('click')
 
         const updatedItems = wrapper.find('MenuItem')
 

diff --git a/test/specs/components/Menu/MenuItem-test.tsx b/test/specs/components/Menu/MenuItem-test.tsx
@@ -5,7 +5,11 @@ import { getTestingRenderedComponent } from 'test/utils'
 import MenuItem from 'src/components/Menu/MenuItem'
 
 describe('MenuItem', () => {
-  isConformant(MenuItem)
+  isConformant(MenuItem, {
+    eventTargets: {
+      onClick: 'a',
+    },
+  })
   handlesAccessibility(MenuItem, { defaultRootRole: 'presentation' })
   handlesAccessibility(MenuItem, { defaultRootRole: 'menuitem', partSelector: 'a' })