Skip to content

Commit

Permalink
feat(menu): memoise menu's childrenToRender function and add valid ro…
Browse files Browse the repository at this point in the history
…le checks (#1539)

* feat: optimise menu and add valid role checks

- memoise menu's childrenToRender function
- add comments explaining menu child checks and the props passed to them
- avoid passing custom props to native HTML elements
- move separator role to the divider component
- check if role exists on menu children and log a console warning if not
- update tests

* fix(lint): lint issue in master

---------

Co-authored-by: Mozafar Haider <[email protected]>
  • Loading branch information
d-rita and kabaros authored Jul 3, 2024
1 parent b24e0f5 commit bddbdae
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 48 deletions.
4 changes: 2 additions & 2 deletions components/calendar/src/stories/calendar-input.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const buildCalendar =
date={date}
locale={locale}
calendar={calendar}
onDateSelect={()=>{}}
onDateSelect={() => {}}
/>
)

Expand Down Expand Up @@ -86,7 +86,7 @@ export const IslamicWithArabic = () => {
locale="ar-SD"
calendar="islamic-civil"
clearable={true}
onDateSelect={()=>{}}
onDateSelect={() => {}}
/>
</div>
)
Expand Down
2 changes: 1 addition & 1 deletion components/divider/src/divider.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const flipMargin = (margin) => {

const Divider = ({ className, dataTest, dense, margin }) => {
return (
<div className={className} data-test={dataTest}>
<div className={className} data-test={dataTest} role="separator">
<style jsx>{`
div {
display: inline-block;
Expand Down
2 changes: 1 addition & 1 deletion components/menu/src/menu-divider/menu-divider.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import PropTypes from 'prop-types'
import React from 'react'

const MenuDivider = ({ className, dataTest, dense }) => (
<li className={className} data-test={dataTest} role="separator">
<li className={className} data-test={dataTest}>
<Divider dense={dense} />

<style jsx>{`
Expand Down
2 changes: 1 addition & 1 deletion components/menu/src/menu/__tests__/menu.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('Menu Component', () => {

expect(menuItem.childAt(0).props().role).toBe('menuitem')
expect(menuItem.childAt(0).prop('aria-label')).toBe('Menu item')
expect(menuDivider.prop('role')).toBe('separator')
expect(menuDivider.find('[role="separator"]').exists()).toBe(true)
})

it('Empty menu has role menu', () => {
Expand Down
14 changes: 12 additions & 2 deletions components/menu/src/menu/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,19 @@ const isValidMenuItemNode = (node) => {
if (node.nodeName === 'LI' && node.firstElementChild) {
return isValidMenuItemNode(node.firstElementChild)
}

const role = node.getAttribute('role')
return role && isMenuItem(role)

// for h1 - h6 headings since their heading role is not explicitly set
// style elements do not have roles
if (node.nodeName.startsWith('H') || node.nodeName === 'STYLE') {
return false
}

if (role) {
return isMenuItem(role)
} else {
console.warn('Missing: role attribute on the menu child')
}
}

export const getFocusableItemsIndices = (elements) => {
Expand Down
88 changes: 47 additions & 41 deletions components/menu/src/menu/menu.js
Original file line number Diff line number Diff line change
@@ -1,53 +1,59 @@
import PropTypes from 'prop-types'
import React, { Children, cloneElement, isValidElement } from 'react'
import React, { Children, cloneElement, isValidElement, useMemo } from 'react'
import { hasMenuItemRole } from './helpers.js'
import { useMenuNavigation } from './use-menu.js'

const Menu = ({ children, className, dataTest, dense }) => {
const { menuRef, focusedIndex } = useMenuNavigation(children)

const childrenToRender = Children.map(children, (child, index) => {
if (!isValidElement(child)) {
return child
}
const tabIndex = index === focusedIndex ? 0 : -1

const childProps = {
...child.props,
}
const childrenToRender = useMemo(
() =>
Children.map(children, (child, index) => {
if (!isValidElement(child)) {
return child
}
const tabIndex = index === focusedIndex ? 0 : -1

if (typeof child.type === 'string') {
// remove non-native props from native HTML elements
delete childProps.hideDivider
delete childProps.dense
delete childProps.active
const childProps = {
...child.props,
}

// all ul children must be li elements
// add tabindex for focus to those elements that are/contain a menuitem
if (child.type === 'li') {
return hasMenuItemRole(child.props.children[0])
? cloneElement(child, { ...childProps, tabIndex })
: cloneElement(child, childProps)
} else {
return (
<li tabIndex={hasMenuItemRole(child) ? tabIndex : null}>
{cloneElement(child, childProps)}
</li>
)
}
} else {
// assign non-native props to custom elements
childProps.dense =
typeof child.props.dense === 'boolean'
? child.props.dense
: dense
childProps.hideDivider =
typeof child.props.hideDivider !== 'boolean' && index === 0
? true
: child.props.hideDivider
return cloneElement(child, { ...childProps, tabIndex })
}
})
// this check is based on the type of child.
// if it is a native HTML element, like li, a, span, only apply its child props
// if it is a functional (React) component, it applies custom props, like dense, hideDivider, etc
if (typeof child.type === 'string') {
// if the native HTML element child is not li, then wrap it in an li tag
// apply the tabindex prop if a child has the menuitem role to make it focusable
if (child.type === 'li') {
return hasMenuItemRole(child.props.children[0])
? cloneElement(child, { ...childProps, tabIndex })
: cloneElement(child, childProps)
} else {
return (
<li
tabIndex={
hasMenuItemRole(child) ? tabIndex : null
}
>
{cloneElement(child, childProps)}
</li>
)
}
} else {
childProps.dense =
typeof child.props.dense === 'boolean'
? child.props.dense
: dense
childProps.hideDivider =
typeof child.props.hideDivider !== 'boolean' &&
index === 0
? true
: child.props.hideDivider
return cloneElement(child, { ...childProps, tabIndex })
}
}),
[children, dense, focusedIndex]
)

return (
<ul
Expand Down
1 change: 1 addition & 0 deletions components/menu/src/menu/use-menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export const useMenuNavigation = (children) => {
const [activeItemIndex, setActiveItemIndex] = useState(-1)

// Initializes the indices for focusable items
// focusable items have the role of menuitem || menuitemcheckbox || menuitemradio
useEffect(() => {
if (menuRef) {
const menuItems = Array.from(menuRef.current.children)
Expand Down

0 comments on commit bddbdae

Please sign in to comment.