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(FilterableMultiSelect): call onMenuChange when mouse click outside #17136

Conversation

gi
Copy link
Contributor

@gi gi commented Aug 9, 2024

This fixes an issue where a mouse click event outside of the menu does not call the onMenuChange callback when the menu is open.

Closes #16896

Changelog

Changed

  • FilterableMultiSelect: calls onMenuChange when mouse click outside of menu

Testing / Reviewing

Unit tests added.

This fixes an issue where a mouse click event outside of the menu
does not call the `onMenuChange` callback when the menu is open.

Fixes carbon-design-system#16896
@gi gi requested a review from a team as a code owner August 9, 2024 22:56
@gi gi requested review from tay1orjones and guidari August 9, 2024 22:56
Copy link

netlify bot commented Aug 9, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 0be9b5b
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66e1d21d42fe180008a85857
😎 Deploy Preview https://deploy-preview-17136--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Aug 9, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 0be9b5b
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66e1d21ded126e0008297674
😎 Deploy Preview https://deploy-preview-17136--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Gururajj77
Copy link
Contributor

Hey @gi , could you please create a test story to test this fix?

This adds an action recorder for `onMenuChange` in storybook.
@gi
Copy link
Contributor Author

gi commented Aug 12, 2024

@Gururajj77 Sure thing! I added an action to be recorded in the Filterable story just like the current onChange action does.

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for doing that!

@gi
Copy link
Contributor Author

gi commented Sep 3, 2024

@tay1orjones Do you have a minute to review this three-line change?

It's blocking my team from upgrading past @carbon/[email protected].

@gi
Copy link
Contributor Author

gi commented Sep 9, 2024

The CI failure just requires that visual changes be reviewed/approved.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Sorry for the huge delay on this, LGTM!

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.86%. Comparing base (cfba956) to head (0be9b5b).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17136      +/-   ##
==========================================
+ Coverage   76.83%   76.86%   +0.02%     
==========================================
  Files         408      408              
  Lines       13949    13949              
  Branches     4276     4275       -1     
==========================================
+ Hits        10718    10722       +4     
+ Misses       3058     3054       -4     
  Partials      173      173              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gi
Copy link
Contributor Author

gi commented Sep 11, 2024

@tay1orjones @guidari Thank you for the reviews and approvals! Can either one of you or other maintainer merge this? I don't think I can do that. (Maybe you have a strategy for this, so sorry if I'm jumping the line.)

@tay1orjones tay1orjones added this pull request to the merge queue Sep 12, 2024
Merged via the queue into carbon-design-system:main with commit 5c70961 Sep 12, 2024
35 checks passed
@gi gi deleted the issue-16896/filterable-multiselect-onmenuchange branch September 12, 2024 23:41
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.

[Bug]: FilterableMultiSelect onMenuChange not called on click outside menu
4 participants