-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(menu): remove unnecessary outside click handler #9528
fix(menu): remove unnecessary outside click handler #9528
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: e1027c6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6127aa1c2ae9c300086caf51 😎 Browse the preview: https://deploy-preview-9528--carbon-react-next.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: e1027c6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6127aa1c5f65020008633f99 😎 Browse the preview: https://deploy-preview-9528--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: e1027c6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6127aa1c93c8910007b764b2 😎 Browse the preview: https://deploy-preview-9528--carbon-components-react.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things look good with react@16.x
I did a yarn upgrade-interactive
and pulled packages/react
react/react-dom deps up to v17.0.2
and re-ran storybook. Things look good there too in both safari and chrome. 👍
One thing I did notice in the overflow menu story -- when it opens there is a brief flash of hover/red on the danger item at the bottom. Here's a video, it might help to download it and play at half speed to see it. It's very fast.
flash.of.red.mov
@tay1orjones Interesting... 🤔 It might have to do with the absolute positioning of the menu, since it theoretically first spawns at x0, y0 before being placed accordingly. If your cursor is on the bottom half of the trigger in this story it would align with the "Delete app" menu item (when the menu is at x0, y0). If you place your cursor on the top half of the trigger, the "Edit routes and access" also gets a hover style though it's far less noticeable since it's just a slight gray and not red. I'll look further into this and try to fix it in a different PR when I get the chance! Thanks for noticing! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! :)
With React v17, the OverflowMenuNext (v11) component wasn't opening anymore supposedly due to smarter execution of the hooks which led to
canBeClosed
beingtrue
immediately when opened, thus allowing theClickListener
to fire itsonOutsideClick
event and closing the menu since the registered click was still on the trigger element.A somewhat recent update to the Menu component added an
onBlur
event in order to correctly return focus for keyboard users. After some testing I noticed that it renders theClickListener
unnecessary as it more accurately detects when the menu should close in this context. Removing theClickListener
and shifting therootRef
on the<ul>
element promises to reduce complexity and fixes the original issue of the menu not opening (more accurately: closing immediately) on React v17.Changelog
Changed
rootRef
to<ul>
elementRemoved
ClickListener
wrapper onMenu
autoclose
prop (not a breaking change since it's marked as experimental/unstable)Testing / Reviewing