fix(dropdown-menu): Remove custom link behavior #90713
Conversation
github-actions
bot
added
the
Scope: Frontend
| &:focus, | ||
| &:focus-visible { | ||
| color: ${getTextColor}; | ||
| box-shadow: none; |
There was a problem hiding this comment.
These overrides are necessary because InnerWrap now gets focus instead of MenuItemWrap
There was a problem hiding this comment.
please make sure you do the same changes in the chonk version if they are needed there as well:
| }); | ||
|
|
||
| // JSDOM throws an error on navigation to random urls | ||
| expect(errorSpy).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
There is already another test in this file which expects this error to be thrown, this already appeared to be an issue
| &:focus, | ||
| &:focus-visible { | ||
| color: ${getTextColor}; | ||
| box-shadow: none; |
There was a problem hiding this comment.
please make sure you do the same changes in the chonk version if they are needed there as well:
| hoverProps, | ||
| keyboardProps, | ||
| makeInnerWrapProps(), | ||
| {ref: innerWrapRef, 'data-test-id': key} |
There was a problem hiding this comment.
why is that data-test-id needed? As far as I can see, all tests use a role-based selector (which is the preferred solution anyway)
There was a problem hiding this comment.
Unfortunately there are quite a few tests using the dropdown component which do rely on the data-test-id 😞
I agree that using roles is better (and the vast majority do that), but I didn't want to convert those tests in this PR
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Currently we have custom logic in the DropdownMenu component which handles link items. The react-aria library should already be handling this, but we configured things a bit differently than react-aria expected which prevented it. This PR removes the custom behavior and provides a `href` for react-aria to use. I'm hoping that this change fixes some bugs I've seen on mobile devices. On iOS especially, taps on dropdown link items have no effect. Summary of changes: - Add `href` to the dropdown item state if `to` or `externalHref` is passed - Remove custom logic handling keyboard selection of a link item - Moves the dropdown item props from the container `li` element to the "inner item" (which is an anchor tag in the case of links) - Fixes tests that looked for `link` roles instead of `menuitemradio`
3 participants
Currently we have custom logic in the DropdownMenu component which handles link items. The react-aria library should already be handling this, but we configured things a bit differently than react-aria expected which prevented it. This PR removes the custom behavior and provides a
hreffor react-aria to use.I'm hoping that this change fixes some bugs I've seen on mobile devices. On iOS especially, taps on dropdown link items have no effect.
Summary of changes:
hrefto the dropdown item state iftoorexternalHrefis passedlielement to the "inner item" (which is an anchor tag in the case of links)linkroles instead ofmenuitemradio