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.

Sign up for GitHub

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

Jump to bottom

feat(filterable-multiselect): add onMenuChange event #7370

Merged
merged 14 commits into from
Dec 4, 2020
Merged

feat(filterable-multiselect): add onMenuChange event #7370

merged 14 commits into from
Dec 4, 2020

Conversation

bstncartwright
Copy link
Contributor

Closes #6434

This is very similar to #7288, but instead adds the onMenuChange event functionality to the FilterableMultiSelect component.

Changelog

New

  • onMenuChange event on filterable multiselect

Changed

Removed

Testing / Reviewing

  • Start storybook
  • Open filterable multiselect story
  • Open the multiselect menu
  • See onMenuChange: (1) [true]
  • Close the multiselect menu
  • See onMenuChange: (1) [false]

@bstncartwright bstncartwright requested review from a team as code owners November 30, 2020 22:01
@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-elements ready!

Built with commit 64e64c2

https://deploy-preview-7370--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 64e64c2

https://deploy-preview-7370--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020 •
edited
Loading

✔️ Deploy preview for carbon-elements ready!

🔨 Explore the source changes: cdf98eb

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/5fca5ea4e441a800082acfa2

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

@netlify
Copy link

netlify bot commented Nov 30, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 85fd9cf

https://deploy-preview-7370--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Nov 30, 2020 •
edited
Loading

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

🔨 Explore the source changes: cdf98eb

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-components-react/deploys/5fca5ea46f61cb0008c7906c

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

@tw15egan
Copy link
Member

tw15egan commented Dec 1, 2020 •
edited
Loading

Thanks for contributing @munkurious!

Do we want onMenuChange to be called when the menu is clicked but is already open? Seems like it is also being called when text is entered, but the menu hasn't changed

filterable-multi

@bstncartwright
Copy link
Contributor Author

Great catch @tw15egan, thanks! I've fixed most of those issues but I am still having one that I am not sure how to resolve due to my inexperience with downshift.

It seems that when you click out of the multiselect, it calls onMenuChange with false twice. I've been able to diagnose which lines are causing them:
https://github.com/munkurious/carbon/blob/e8d02129305c39c897dfdc47e7d2975ffb601137/packages/react/src/components/MultiSelect/FilterableMultiSelect.js#L206
and
https://github.com/munkurious/carbon/blob/e8d02129305c39c897dfdc47e7d2975ffb601137/packages/react/src/components/MultiSelect/FilterableMultiSelect.js#L228

When clicking out of the multi select, the handleOnStateChange function and the handleOnOuterClick function gets called. I'm not sure of the best way to handle this and am welcome to any ideas :)

emyarod
emyarod reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment •
edited
Loading

Choose a reason for hiding this comment

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

expand to view GIF

the onMenuChange event seems to fire twice when collapsing the menu

edit: looks like you're aware of this already, I started recording this while you were writing your comment above

@bstncartwright @emyarod
Update packages/react/src/components/MultiSelect/FilterableMultiSelec…
671d5db 
…t.js

Co-authored-by: emyarod <emyarod@users.noreply.github.com>
@bstncartwright
Copy link
Contributor Author

Awesome, thanks for your help! It seems to do the job 👍

emyarod
emyarod approved these changes Dec 4, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me

tw15egan
tw15egan approved these changes Dec 4, 2020
View reviewed changes
Copy link
Member

@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.

LGTM, thanks for contributing! 👍 ✅

@kodiakhq kodiakhq bot merged commit 27ce461 into carbon-design-system:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod approved these changes

@tw15egan tw15egan tw15egan approved these changes

@joshblack joshblack Awaiting requested review from joshblack

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Filterable Multiselect onClose
3 participants
@bstncartwright @tw15egan @emyarod