Skip to content
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(Combobox): add event propagation to mouse down #9402

Merged

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Aug 3, 2021

Closes #9073

  • removes event.stopPropagation() from the onMouseUp event on the toggle button's props.
  • onMouseUp on the toggle button now moves active focus to the text input

Testing / Reviewing

  • go into ../ComboBox-story.js and duplicate the <ComboBox> seen on line 65 like so
    image
  • open the second ComboBox's listbox either by clicking the text input or the toggle button
  • click the first ComboBox's toggle button and observe the second Combobox's listbox close as expected

Notes

@joshblack pretty sure stopping event propagation was critical for some reason we're missing here -- do you remember why?

@dakahn dakahn requested a review from joshblack August 3, 2021 21:08
@dakahn dakahn requested a review from a team as a code owner August 3, 2021 21:08
@dakahn dakahn requested a review from tw15egan August 3, 2021 21:08
@netlify
Copy link

netlify bot commented Aug 3, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 405c5d2

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6130c0c72a0aec0007226e2d

😎 Browse the preview: https://deploy-preview-9402--carbon-react-next.netlify.app/

@netlify
Copy link

netlify bot commented Aug 3, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 405c5d2

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6130c0c7e63d5e00086e61c5

😎 Browse the preview: https://deploy-preview-9402--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 3, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 405c5d2

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6130c0c70b603900075f6b3c

😎 Browse the preview: https://deploy-preview-9402--carbon-components-react.netlify.app

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working as expected 👍🏻 ✅

@tay1orjones
Copy link
Member

I debugged this at length with @sstrubberg today, and it seems just scoping the event.stopPropogation to only when the listbox isOpen fixes everything. I can no longer get two listboxes to be open at the same time, and the bug @joshblack pointed out on the single combobox is also fixed.

only.one.open.at.a.time.mov

I added a multipleInstances story to demo this. I can take it out, but sort of feel it's nice to have and we could leave in? I'm good either way.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed and removed the test story, LGTM 👍🏻

@kodiakhq kodiakhq bot merged commit 2dd2b31 into carbon-design-system:main Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ComboBox not dismissing other open ComboBox or DropDown
4 participants