fix(menu): correctly identify menu item link with same URL as web page#693
fix(menu): correctly identify menu item link with same URL as web page#693
Conversation
jzempel
left a comment
jzempel
left a comment
There was a problem hiding this comment.
Some 🐛 ...
- with the uncontrolled menu story:
- Click "Aloe Vera" link in the story
- Use back button to return to the story
- Click "Aloe Vera" link again
Result: nothing happens
- with the controlled menu story:
- Change "Aloe Vera" to
isExternal={true} - Click the "Aloe Vera" link
Result: nothing happens
- adding
selectedto the "Aloe Vera" item (and remounting) doesn't work as expected. Items withhrefshould be able to be initialized selected outside the context of an item group.
| <a | ||
| {...(itemProps as AnchorHTMLAttributes<HTMLAnchorElement>)} | ||
| className="w-full rounded-sm outline-offset-0 transition-none border-width-none" | ||
| href={item.disabled ? undefined : itemProps.href} |
There was a problem hiding this comment.
This href prop manipulation should be happening within the hook so that spreading ...itemProps Just Works ™️
packages/menu/src/useMenu.ts
Outdated
| elementProps.href = href; | ||
|
|
||
| if (!itemDisabled) { | ||
| if (isExternal) { |
There was a problem hiding this comment.
Should probably be named external to keep parity with the other aspects of the container API (selected, disabled). Compare with other container APIs – we don't enforce the isXxx naming like we do in react-components. Also, the documentation for item.href and item.external is missing from the types interface.
jzempel
left a comment
jzempel
left a comment
There was a problem hiding this comment.
Approving as these items can either be addressed here in this PR or in a follow-on.
| role={item.href ? 'none' : undefined} | ||
| {...(!item.href && (itemProps as LiHTMLAttributes<HTMLLIElement>))} | ||
| > | ||
| {itemProps.href ? ( | ||
| {item.href ? ( | ||
| <a | ||
| {...(itemProps as AnchorHTMLAttributes<HTMLAnchorElement>)} |
There was a problem hiding this comment.
Hmm, I don't want to block this PR, but normally a container would expose a prop spread per element. So depending on whether there's a defined href, the hook would produce itemProps and defined/undefined/null anchorProps. This prevents the consumer from needing to go through the logic seen here. I'll continue with an approval review. You can either solve here or we can revisit later.
If you decide to render with a getAnchorProps, please just remember to add it to the Storybook API docs.
There was a problem hiding this comment.
Also, should the README (https://zendeskgarden.github.io/react-containers/?path=/docs/packages-menu--docs#usemenu) be updated with an anchor example? Again, might be a lot cleaner if we wait to have getAnchorProps.
There was a problem hiding this comment.
Is there an item treatment that can be done for anchor items if selected is true? It would be nice to see a visual change here in the containers storybook (as opposed to react-components where we don't want any visual artifact).
|
Closing this stale PR for now. Lets come back to it if needed. |
4 participants
Description
useMenucorrectly identifies the menu item link with the same URL as the current web page.Detail
getItemPropssetsaria-current="page"to the menu item link with the same URL as the current web page.targetandrefattributes.Checklist
npm start)