Feat/dropdown expandable menu #1129
Conversation
🦋 Changeset detectedLatest commit: 55898ea The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
One thing I'm noticing is keyboard navigation feels a little weird because, after I tab into the group, I can use my arrow keys to navigate through both closed "accordions." If I open one, then I lose the ability to navigate to them. Also if they are both open, the arrow keys loop through the open items.
@orion-cengage can you take a look to see how this navigation compares to your expectations? Maybe it's right, but just want another pair of eyes on it.
Also, what happens if the use provides icons for only some of the top level items?
| const testId = 'expandable group'; | ||
| const testId2 = 'expandable item'; | ||
| const testId3 = 'expandable button'; | ||
| const testId4 = 'expandable panel'; |
There was a problem hiding this comment.
Nitpick, but it would be helpful if these variables had useful names for readability:
const expButtonId = 'expandable-button';
const expPanel1Id = 'expandable-panel-1';
You also use ``${testId4}-2} a few times, you could just add another variable for it
| expect(queryByTestId(`${testId4}-2`)).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('should have only allow one open menu item when isMulti is used', () => { |
There was a problem hiding this comment.
Let's have separate tests for when isMulti is true and when isMulti is false (Since isMulti takes a boolean, I personally find it easier to follow the descriptions when we use the exact value you're passing to a prop)
There was a problem hiding this comment.
Anything not declaring the isMulti prop is automatically true. Do we need a specific test for that?
There was a problem hiding this comment.
I would say yes for full coverage.
packages/react-magma-dom/src/components/Dropdown/Dropdown.test.js
Outdated
Show resolved
Hide resolved
| icon?: React.ReactElement<IconProps>; | ||
| }>` | ||
| font-weight: 400; | ||
| padding: 8px 16px; |
| // const i18n = React.useContext(I18nContext); | ||
|
|
||
| return ( | ||
| <StyledAccordionPanel |
There was a problem hiding this comment.
Did you mean {...other}? The other Dropdown components all use that spread operator.
| return ( | ||
| <StyledAccordionPanel | ||
| hasIcon={context.hasIcon} | ||
| isExpandablePanel={isExpandablePanel} |
There was a problem hiding this comment.
Should we be using isExpandablePanel from the DropdownExpandableMenuItemContext or from props?
There was a problem hiding this comment.
Refactored and no longer a prop in this component.
| if (props.hasIcon && props.isExpandablePanel) { | ||
| return `${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing05} ${props.theme.spaceScale.spacing03} 72px`; | ||
| } | ||
| //For DropdownExpandableMenu styling without an icon | ||
| else if (props.isExpandablePanel) { |
There was a problem hiding this comment.
You're checking props.isExpandablePanel twice, and removed the case for padding when it's not expandable. This needs to be updated as it's breaking other dropdowns:
There was a problem hiding this comment.
That actually wasn't removed and still behaves as it did before.
There was a problem hiding this comment.
Not sure what you mean 🤔
There was a problem hiding this comment.
Padding remains at 8px 0 when no DropdownItems exist as it did before.
There was a problem hiding this comment.
Still looks broken to me. Check out the No Items story. This is what it looks like in the dev branch: https://storybook-preview-dev--upbeat-sinoussi-f675aa.netlify.app/?path=/story/dropdown--no-items
| DropdownAlignment, | ||
| DropdownDropDirection, | ||
| DropdownProps, | ||
| } from './components/Dropdown'; | ||
| export { | ||
| DropdownContent, | ||
| DropdownContentProps, | ||
| } from './components/Dropdown/DropdownContent'; | ||
| export { | ||
| DropdownDivider, | ||
| DropdownDividerProps, | ||
| } from './components/Dropdown/DropdownDivider'; | ||
| export { | ||
| DropdownHeader, | ||
| DropdownHeaderProps, | ||
| } from './components/Dropdown/DropdownHeader'; | ||
| export { | ||
| DropdownMenuGroup, | ||
| DropdownMenuGroupProps, | ||
| } from './components/Dropdown/DropdownMenuGroup'; | ||
| export { | ||
| DropdownMenuItem, | ||
| DropdownMenuItemProps, | ||
| } from './components/Dropdown/DropdownMenuItem'; | ||
| export { | ||
| DropdownMenuNavItem, | ||
| DropdownMenuNavItemProps, | ||
| } from './components/Dropdown/DropdownMenuNavItem'; | ||
| export { | ||
| DropdownSplitButton, | ||
| DropdownSplitButtonProps, | ||
| } from './components/Dropdown/DropdownSplitButton'; | ||
| export { | ||
| DropdownButton, | ||
| DropdownButtonProps, | ||
| } from './components/Dropdown/DropdownButton'; |
There was a problem hiding this comment.
Let's make sure all of these components are actually exported in packages/react-magma-dom/src/components/Dropdown/index.tsx if we want to do this (I see that some are not). I think it's okay to import them from their individual files like we were previously doing.
| @@ -127,6 +131,19 @@ export const DropdownContent = React.forwardRef< | |||
| } | |||
| }); | |||
|
|
|||
| // For Expandable Dropdowns that don't require a max-height | |||
| let hasExpandable = false; | |||
There was a problem hiding this comment.
I think this variable name and comment are throwing me off a bit. Could we be more verbose and call it hasExpandableItems or something?? and then the comment can also be updated
…sent, pushing changes for keyboard navigation tweaks
| @@ -174,7 +174,6 @@ export const Dropdown = React.forwardRef<HTMLDivElement, DropdownProps>( | |||
|
|
|||
| function handleKeyDown(event: React.KeyboardEvent) { | |||
| if (event.key === 'Escape') { | |||
| event.nativeEvent.stopImmediatePropagation(); | |||
There was a problem hiding this comment.
Why are we removing this?
There was a problem hiding this comment.
Accidental removal
|
|
||
| const StyledIconWrapper = styled(IconWrapper)` | ||
| justify-content: center; | ||
| /* align-items: center; */ |
| <StyledAccordion | ||
| {...other} | ||
| isInverse={context.isInverse} | ||
| role="expandable group" |
There was a problem hiding this comment.
This is not a valid aria role
There was a problem hiding this comment.
Changed to role="group"
| const { children, disabled, testId, ...other } = props; | ||
|
|
||
| return ( | ||
| <AccordionItem isDisabled={disabled} {...other} testId={testId}> |
There was a problem hiding this comment.
| <AccordionItem isDisabled={disabled} {...other} testId={testId}> | |
| <AccordionItem {...other} isDisabled={disabled} testId={testId}> |
| ` | ||
| -------------------------------------------------------------------------------------------------------- | ||
| Only one group level is supported, anything nested two levels or more isn't accounted for in the styling | ||
| -------------------------------------------------------------------------------------------------------- | ||
| ` |
There was a problem hiding this comment.
For consistency with our other warnings,
| ` | |
| -------------------------------------------------------------------------------------------------------- | |
| Only one group level is supported, anything nested two levels or more isn't accounted for in the styling | |
| -------------------------------------------------------------------------------------------------------- | |
| ` | |
| ` | |
| React Magma Warning: Only one group level is supported for Expandable Dropdowns, anything nested two levels or more isn't accounted for in the styling | |
| ` |
packages/react-magma-dom/src/components/Dropdown/DropdownExpandableMenuItem.tsx
Show resolved
Hide resolved
| import { DropdownContext } from './Dropdown'; | ||
|
|
||
| const StyledAccordion = styled(Accordion)<{ | ||
| testId?: string; |
There was a problem hiding this comment.
This testId is not needed here
packages/react-magma-dom/src/components/Dropdown/DropdownExpandableMenuGroup.tsx
Show resolved
Hide resolved
| }); | ||
|
|
||
| return ( | ||
| <DropdownExpandableContext.Provider value={{ hasIcon, isExpandablePanel }}> |
There was a problem hiding this comment.
| <DropdownExpandableContext.Provider value={{ hasIcon, isExpandablePanel }}> | |
| <DropdownExpandableMenuGroupContext.Provider value={{ hasIcon, isExpandablePanel }}> |
|
|
||
| const context = React.useContext(DropdownContext); | ||
|
|
||
| let hasIcon = false; |
There was a problem hiding this comment.
I tend to prefer long variable names that are very clear over short ones that are less descriptive. In this case, the MenuGroup wouldn't have icons-- the children menu button is what we are after. Maybe we can call this expandableMenuButtonHasIcon or something like that.
| @@ -129,6 +146,8 @@ export const DropdownMenuItem = React.forwardRef< | |||
| const theme = React.useContext(ThemeContext); | |||
| const context = React.useContext(DropdownContext); | |||
|
|
|||
| const expandableContext = React.useContext(DropdownExpandableContext); | |||
There was a problem hiding this comment.
| const expandableContext = React.useContext(DropdownExpandableContext); | |
| const menuGroupContext = React.useContext(DropdownExpandableMenuGroupContext); |
You may also check if it's expandable with const isExpandablePanel = !!menuGroupContext; instead of getting it from the context.
There was a problem hiding this comment.
Seems that the context is necessary here.
… amendments per Laura's review, still working on tests
packages/react-magma-dom/src/components/Dropdown/DropdownExpandableMenuGroup.tsx
Show resolved
Hide resolved
packages/react-magma-dom/src/components/Accordion/AccordionButton.tsx
Outdated
Show resolved
Hide resolved
packages/react-magma-dom/src/components/Accordion/AccordionButton.tsx
Outdated
Show resolved
Hide resolved
packages/react-magma-dom/src/components/Accordion/AccordionButton.tsx
Outdated
Show resolved
Hide resolved
| @@ -23,10 +23,12 @@ export interface AccordionButtonProps | |||
| /** | |||
| * @internal | |||
| */ | |||
| hasCustomArray?: boolean; | |||
There was a problem hiding this comment.
This name is really ambiguous. Can we think of something more specific and descriptive? Lets also add a comment for further clarity on how this prop should be used
packages/react-magma-dom/src/components/Dropdown/DropdownExpandableMenuButton.tsx
Outdated
Show resolved
Hide resolved
packages/react-magma-dom/src/components/Dropdown/DropdownExpandableMenuPanel.tsx
Outdated
Show resolved
Hide resolved
| function menuItemPadding(props) { | ||
| //For DropdownExpandableMenu styling with an icon | ||
| if (props.expandableMenuButtonHasIcon && props.isExpandablePanel) { | ||
| return `${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing05} ${props.theme.spaceScale.spacing03} 72px`; | ||
| } | ||
| //For DropdownExpandableMenu styling without an icon | ||
| else if (props.isExpandablePanel) { | ||
| return `${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing05} ${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing08}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be simplified now:
function menuItemPadding(props) {
if (props.isExpandablePanel) {
if (props.expandableMenuButtonHasIcon) {
return `${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing05} ${props.theme.spaceScale.spacing03} 72px`;
}
return `${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing05} ${props.theme.spaceScale.spacing03} ${props.theme.spaceScale.spacing08}`;
}
}
| ## Dropdown Expandable Menu List Item Props | ||
|
|
||
| **Any other props supplied will be provided to the wrapping `div` element.** | ||
|
|
||
| <DropdownExpandableMenuListItemProps /> | ||
|
|
There was a problem hiding this comment.
This looks right, but for whatever reason this props table is not showing up in the docs site:
| &:hover, | ||
| &:focus { | ||
| background: ${menuBackground}; | ||
| } |
There was a problem hiding this comment.
Is this still needed? It may already default to the correct styles from DropdownMenuItem
There was a problem hiding this comment.
Removed.
| isFixedWidth={context.isFixedWidth} | ||
| isInverse={context.isInverse} | ||
| ref={disabled ? null : ref} | ||
| role="menuitem" |
There was a problem hiding this comment.
Same with some of these, are they necessary?
There was a problem hiding this comment.
Removed redundancies
| disabled?: boolean; | ||
| expandableMenuButtonHasIcon?: boolean; | ||
| isExpandablePanel?: boolean; | ||
| isFixedWidth?: boolean; | ||
| isInverse?: boolean; |
There was a problem hiding this comment.
Some of these are not needed as they are not used by menuItemPadding
|
This is basically good to go, I think it can be marked as ready for review 😄 |
chris-cedrone-cengage
added
the
needs design feedback
Issue: # 1014
What I did
-New sub-component of Dropdown that allows expanding menu items, with or without icons at the parent level.
Screenshots:
Checklist
How to test