Repository: uber-web/baseui
Pull request #247 (2018-09-14 07:42:39)

fix(menu): address Menu todos; fix types; better a11y
### Menu Component

Addressing the TODOs in https://github.com/uber-web/baseui/issues/185 as well as pulling in the `Menu` changes from the `Pagination` PRs since those might take some more time to land.


Patch:
diff --git a/src/form-control/form-control.js b/src/form-control/form-control.js
index 96232b6bc..ae3fe15ea 100644
--- a/src/form-control/form-control.js
+++ b/src/form-control/form-control.js
@@ -14,7 +14,7 @@ import {
 } from './styled-components';
 import type {FormControlPropsT} from './types';
 
-function mapStyletronProps(props: {}, mapper: {}) {
+function getSharedProps(props: {}, mapper: {}) {
   return Object.keys(props).reduce((newProps, propName) => {
     const newName = mapper[propName] && `$${propName}`;
     if (newName) {
@@ -42,10 +42,7 @@ export default class FormControl extends React.Component<FormControlPropsT> {
     } = this.props;
 
     const onlyChildProps = React.Children.only(children).props;
-    const sharedProps = mapStyletronProps(
-      onlyChildProps,
-      STYLETRON_PROP_MAPPER,
-    );
+    const sharedProps = getSharedProps(onlyChildProps, STYLETRON_PROP_MAPPER);
     sharedProps.$error = this.props.error || sharedProps.$error;
     const Label = getOverride(LabelOverride) || StyledLabel;
     const Caption = getOverride(CaptionOverride) || StyledCaption;
diff --git a/src/menu/constants.js b/src/menu/constants.js
index 8d4e50e19..27fa62807 100644
--- a/src/menu/constants.js
+++ b/src/menu/constants.js
@@ -13,7 +13,7 @@ export const STATE_CHANGE_TYPES = {
 };
 
 // Dict of props to prepend $ to pass thru to styletron
-export const STYLETRON_PROP_MAPPER = {
+export const SHARED_PROPS_MAPPER = {
   ref: true,
   isHighlighted: true,
 };
diff --git a/src/menu/menu.js b/src/menu/menu.js
index 5c1956250..b47f0b2fc 100644
--- a/src/menu/menu.js
+++ b/src/menu/menu.js
@@ -9,14 +9,14 @@ import * as React from 'react';
 // Components
 import {List as StyledList} from './styled-components';
 import OptionList from './option-list';
-import {mapStyletronProps} from './utils';
+import {getSharedProps} from './utils';
 import {getOverrideObject} from '../helpers/overrides';
 // Types
 import type {StatelessMenuPropsT} from './types';
 
 export default function Menu({
   items,
-  getRequiredItemProps = (item, index) => ({key: String(index)}),
+  getRequiredItemProps = (item, index) => ({}),
   rootRef = React.createRef(),
   overrides = {},
 }: StatelessMenuPropsT) {
@@ -32,12 +32,14 @@ export default function Menu({
     <List $ref={rootRef} {...listProps}>
       {items.map((item, index) => {
         const requiredProps = getRequiredItemProps(item, index);
-        // Need to be explicit with `key` otherwise eslint throws error?
         return (
-          // eslint-disable-next-line react/jsx-key
           <Option
+            key={index}
             item={item}
-            {...mapStyletronProps(requiredProps)}
+            role="option"
+            // Allows tab focus into first element
+            tabIndex={index === 0 ? 0 : -1}
+            {...getSharedProps(requiredProps)}
             {...optionProps}
           />
         );
diff --git a/src/menu/stateful-container.js b/src/menu/stateful-container.js
index c88d9f16c..063bc22a0 100644
--- a/src/menu/stateful-container.js
+++ b/src/menu/stateful-container.js
@@ -12,32 +12,35 @@ import {scrollItemIntoView} from './utils';
 // Types
 import type {
   StatefulContainerPropsT,
-  DefaultStatefulContainerPropsT,
   StatefulContainerStateT,
   GetRequiredItemPropsFnT,
   RootRefT,
   RenderPropsT,
+  StateReducerFnT,
 } from './types';
 
 export default class MenuStatefulContainer extends React.Component<
   StatefulContainerPropsT,
   StatefulContainerStateT,
 > {
-  static defaultProps: DefaultStatefulContainerPropsT = {
+  static defaultProps = {
     initialState: {
       // We start the index at -1 to indicate that no highlighting exists initially
       highlightedIndex: -1,
     },
-    stateReducer: (changeType, changes) => changes,
+    stateReducer: (
+      changeType: ?$PropertyType<StateReducerFnT, 'changeType'>,
+      changes: $PropertyType<StateReducerFnT, 'changes'>,
+    ) => changes,
     onItemSelect: () => {},
-    children: () => {},
+    children: () => null,
   };
 
   state: StatefulContainerStateT = {...this.props.initialState};
 
   componentDidMount() {
     if (__BROWSER__) {
-      // TODO: perhaps only bind event listener on focus
+      // TODO(#185): perhaps only bind event listener on focus
       document.addEventListener('keydown', this.onKeyDown);
     }
   }
@@ -61,7 +64,6 @@ export default class MenuStatefulContainer extends React.Component<
     changes: StatefulContainerStateT,
   ) {
     const {stateReducer} = this.props;
-    // $FlowFixMe
     this.setState(stateReducer(changeType, changes, this.state));
   }
 
@@ -127,11 +129,9 @@ export default class MenuStatefulContainer extends React.Component<
       this.refList[index] = itemRef;
     }
     return {
-      key: `${index}`,
       ref: itemRef,
       isHighlighted: highlightedIndex === index,
       onClick: onClickHandler,
-      role: 'option',
       'aria-activedescendant': highlightedIndex === index,
     };
   };
@@ -139,7 +139,6 @@ export default class MenuStatefulContainer extends React.Component<
   render() {
     const {highlightedIndex} = this.state;
     const {children, items} = this.props;
-    // $FlowFixMe
     return children(
       ({
         highlightedIndex,
diff --git a/src/menu/stateful-menu.js b/src/menu/stateful-menu.js
index 7d8e1baf6..b0e4f8e18 100644
--- a/src/menu/stateful-menu.js
+++ b/src/menu/stateful-menu.js
@@ -17,6 +17,8 @@ export default class StatefulMenu extends React.PureComponent<
   StatefulMenuPropsT,
 > {
   static defaultProps = {
+    // Mostly to satisfy flow
+    ...StatefulContainer.defaultProps,
     overrides: {},
   };
 
diff --git a/src/menu/styled-components.js b/src/menu/styled-components.js
index 05e9b2516..f101a7923 100644
--- a/src/menu/styled-components.js
+++ b/src/menu/styled-components.js
@@ -24,6 +24,7 @@ export const List = styled('ul', ({$theme}: StyledPropsT) => ({
   background: $theme.colors.white,
   borderRadius: $theme.borders.radius300,
   boxShadow: $theme.lighting.shadow600,
+  overflow: 'auto',
 }));
 
 export const ListItem = styled(
diff --git a/src/menu/types.js b/src/menu/types.js
index cfc8d37d6..53c50634f 100644
--- a/src/menu/types.js
+++ b/src/menu/types.js
@@ -10,36 +10,34 @@ import * as React from 'react';
 import type {OverrideT} from '../helpers/overrides';
 import {STATE_CHANGE_TYPES} from './constants';
 
-export type ItemsT = Array<*>;
+export type ItemT = *;
+export type ItemsT = Array<ItemT>;
 
-export type GetItemLabelFnT = (item: *) => React$Node;
+export type GetItemLabelFnT = (item: ItemT) => React$Node;
 
 export type GetProfileItemLabelsFnT = (
-  item: *,
+  item: ItemT,
 ) => {
   title?: string,
   subtitle?: string,
   body?: string,
 };
 
-export type GetProfileItemImgFnT = (item: *) => string | React.ComponentType<*>;
+export type GetProfileItemImgFnT = (
+  item: ItemT,
+) => string | React.ComponentType<*>;
 
-export type GetProfileItemImgTextFnT = (item: *) => string;
+export type GetProfileItemImgTextFnT = (item: ItemT) => string;
 
 export type SetRootRefFnT = (ref: React$ElementRef<*>) => void;
 
 export type RootRefT = React$ElementRef<*>;
 
 export type OnItemSelectFnT = (
-  item: ?{},
+  item: ?ItemT,
   event: SyntheticEvent<> | KeyboardEvent,
 ) => mixed;
 
-export type OverridesT = {
-  List?: OverrideT<*>,
-  ListItem?: OverrideT<*>,
-};
-
 export type ProfileOverridesT = {
   List?: OverrideT<*>,
   ListItemProfile?: OverrideT<*>,
@@ -52,22 +50,19 @@ export type ProfileOverridesT = {
 };
 
 export type RenderItemPropsT = {
-  key: string,
   ref?: React$ElementRef<*>,
   // indicates when the item is visually focused
   isHighlighted?: boolean,
-  role?: string,
-  // indicates when the item is visually focused
   'aria-activedescendant'?: boolean,
 };
 
 export type GetRequiredItemPropsFnT = (
-  item: {},
+  item: ItemT,
   index: number,
 ) => RenderItemPropsT;
 
 export type StateReducerFnT = (
-  changeType: $Keys<typeof STATE_CHANGE_TYPES>,
+  changeType: ?$Keys<typeof STATE_CHANGE_TYPES>,
   changes: StatefulContainerStateT,
   currentState: StatefulContainerStateT,
 ) => StatefulContainerStateT;
@@ -99,10 +94,10 @@ export type DefaultStatefulContainerPropsT = {
 
 export type StatefulContainerPropsT = {
   items: ItemsT,
-  initialState?: StatefulContainerStateT,
-  stateReducer?: StateReducerFnT,
-  onItemSelect?: OnItemSelectFnT,
-  children?: RenderPropsT => React.Node,
+  initialState: StatefulContainerStateT,
+  stateReducer: StateReducerFnT,
+  onItemSelect: OnItemSelectFnT,
+  children: RenderPropsT => React.Node,
 };
 
 export type MenuPropsT = {
@@ -137,7 +132,7 @@ export type StatelessMenuProfilePropsT = SharedStatelessPropsT &
   MenuProfilePropsT;
 
 export type OptionListPropsT = {
-  item: *,
+  item: ItemT,
   getItemLabel: GetItemLabelFnT,
   overrides?: {
     ListItem?: OverrideT<*>,
@@ -145,7 +140,7 @@ export type OptionListPropsT = {
 };
 
 export type OptionProfilePropsT = {
-  item: *,
+  item: ItemT,
   getProfileItemLabels: GetProfileItemLabelsFnT,
   getProfileItemImg: GetProfileItemImgFnT,
   getProfileItemImgText: GetProfileItemImgTextFnT,
diff --git a/src/menu/utils.js b/src/menu/utils.js
index 33345e372..3fd322250 100644
--- a/src/menu/utils.js
+++ b/src/menu/utils.js
@@ -7,7 +7,7 @@ LICENSE file in the root directory of this source tree.
 // @flow
 /* eslint-disable import/prefer-default-export */
 import smoothscroll from 'smoothscroll-polyfill';
-import {STYLETRON_PROP_MAPPER} from './constants';
+import {SHARED_PROPS_MAPPER} from './constants';
 
 if (__BROWSER__) {
   smoothscroll.polyfill();
@@ -18,9 +18,9 @@ if (__BROWSER__) {
  * all of the existing prop keys inside mapper with $ to present styletron
  * from passing through those props to the underlying React component.
  */
-export function mapStyletronProps(
+export function getSharedProps(
   props: {},
-  mapper: {} = STYLETRON_PROP_MAPPER,
+  mapper: {} = SHARED_PROPS_MAPPER,
 ): {} {
   return Object.keys(props).reduce((newProps, propName) => {
     const newName = mapper[propName] ? `$${propName}` : propName;


Test patch:
diff --git a/src/menu/__tests__/__snapshots__/styled-components.test.js.snap b/src/menu/__tests__/__snapshots__/styled-components.test.js.snap
index 1e3e72b0a..e8de604f3 100644
--- a/src/menu/__tests__/__snapshots__/styled-components.test.js.snap
+++ b/src/menu/__tests__/__snapshots__/styled-components.test.js.snap
@@ -6,6 +6,7 @@ Object {
   "borderRadius": "$theme.borders.radius300",
   "boxShadow": "$theme.lighting.shadow600",
   "margin": 0,
+  "overflow": "auto",
   "padding": 0,
   "position": "relative",
 }
diff --git a/src/menu/__tests__/stateful-container.test.js b/src/menu/__tests__/stateful-container.test.js
index 2ac21db05..73dcf3235 100644
--- a/src/menu/__tests__/stateful-container.test.js
+++ b/src/menu/__tests__/stateful-container.test.js
@@ -91,11 +91,9 @@ describe('Menu StatefulContainer', () => {
     const item = mockItems[0];
     const props = component.instance().getRequiredItemProps(item, 0);
     expect(props).toEqual({
-      key: '0',
       ref: React.createRef(),
       isHighlighted: false,
       onClick: props.onClick,
-      role: 'option',
       'aria-activedescendant': false,
     });
     props.onClick();
@@ -110,11 +108,9 @@ describe('Menu StatefulContainer', () => {
     const item = mockItems[0];
     const props = component.instance().getRequiredItemProps(item, 0);
     expect(props).toEqual({
-      key: '0',
       ref: React.createRef(),
       isHighlighted: true,
       onClick: props.onClick,
-      role: 'option',
       'aria-activedescendant': true,
     });
   });
diff --git a/src/menu/__tests__/utils.test.js b/src/menu/__tests__/utils.test.js
index 35448791e..100e7a9f4 100644
--- a/src/menu/__tests__/utils.test.js
+++ b/src/menu/__tests__/utils.test.js
@@ -7,7 +7,7 @@ LICENSE file in the root directory of this source tree.
 // @flow
 import * as Utils from '../utils';
 
-describe('Menu Utils - mapStyletronProps', () => {
+describe('Menu Utils - getSharedProps', () => {
   test('successfully maps', () => {
     const props = {
       someStyletronProps: 'styletron',
@@ -16,7 +16,7 @@ describe('Menu Utils - mapStyletronProps', () => {
     const mapper = {
       someStyletronProps: true,
     };
-    expect(Utils.mapStyletronProps(props, mapper)).toEqual({
+    expect(Utils.getSharedProps(props, mapper)).toEqual({
       $someStyletronProps: 'styletron',
       normalProps: 'normal',
     });