Skip to content
This repository has been archived by the owner on Mar 8, 2023. It is now read-only.

feat(multi-select): introduce filterable mode #716

Merged

Conversation

IgnacioBecerra
Copy link
Contributor

Related Ticket(s)

#683

Description

Introduced a filterable variant of the Multi Select component that works as a ComboBox that filters and is able to select multiple options.

Changelog

New

  • new filterable variant for bx-multi-select

@IgnacioBecerra IgnacioBecerra requested a review from a team as a code owner October 23, 2021 00:19
@IgnacioBecerra IgnacioBecerra requested review from emyarod, ariellalgilmore and oliviaflory and removed request for a team October 23, 2021 00:19
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 23, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 23, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 23, 2021

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 23, 2021

@jeffchew
Copy link
Member

Thank you @IgnacioBecerra!

It looks like some of the unit tests are failing:

image

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.

it looks like events continue to propagate after typing. In this screen recording you can see the storybook UI being toggled as I type T, and the multiselect menu does not stay open and filter while typing 1 for example

Screen.Recording.2021-10-26.at.9.26.54.AM.mov

@IgnacioBecerra
Copy link
Contributor Author

@emyarod Seems like this can also be seen in the ComboBox component, seems like the 1 is a keyboard shortcut to focus on the Storybook component list sidebar. Tried using event.stopPropagation() but it seems that this shortcut takes precedence over the other functions and can't disable it. Maybe we could open another issue for this to investigate? The component works fine if opened in a new tab.

@emyarod
Copy link
Member

emyarod commented Oct 27, 2021

@IgnacioBecerra I think we should be able to ignore storybook shortcuts given that the core Carbon filterable multiselect story doesn't run into this issue right?

I agree on opening a separate issue for the existing ComboBox web component, but for a new variant story I think we should resolve before merging if we can, what do you think? interested in hearing others' opinions as well

@IgnacioBecerra
Copy link
Contributor Author

@emyarod I noticed that BXSearch also has this weird behavior while BXInput didn't. I tried to see what's the difference between these two components and kept adding/removing things, I decided to copy the BXInput code into BXSearch only to find out the behavior still occurred. I stripped down both components to only use an input element in the shadow DOM for both components, got rid of the CSS styles, and even modified the storybook files to be exactly the same... and the behavior is still there for BXSearch. Not sure what's going on lol

I opened a draft PR showing this behavior, and you can see the files are carbon ( heh ) copies of each other:
#717

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.

should we also add the stories to the React/Vue/Angular storybooks?

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.

one thing with Space selection, when there is an active cursor in the input we end up with some possibly unexpected behavior (the menu closes vs when no cursor is active)

Screen.Recording.2021-10-29.at.10.04.49.AM.mov

it looks like this behavior is present in the core component so we may need further guidance on what the expected behavior should be

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!

@IgnacioBecerra IgnacioBecerra added the Ready to merge Label for the pull requests that are ready to merge label Nov 5, 2021
@kodiakhq kodiakhq bot merged commit 43af386 into carbon-design-system:master Nov 10, 2021
@abdonrd
Copy link
Collaborator

abdonrd commented Nov 10, 2021

@IgnacioBecerra @jeffchew seems like we need to add a new snapshot, right?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants