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

feat(filterable-select): add disableDefaultFiltering prop #6469

Merged

Conversation

stripeyjumper
Copy link
Contributor

@stripeyjumper stripeyjumper commented Dec 5, 2023

Proposed behaviour

Adds a new boolean prop to the FilterableSelect component, disableDefaultFiltering.

  • By default disableDefaultFiltering is false, and filtering and highlighting of options is handled by the component itself.
  • If disableDefaultFiltering is set to true, the component does not automatically filter or highlight options as the user enters a text filter.

This allows use-cases like server-side filtering of options, or rich formatting of options. Our use-case is an address search dropdown.

The new implementation uses a FilterableSelectList by default, and a SelectList if disableDefaultFiltering is true. It is still appropriate to use a FilterableSelect in this case, because we still want access to the filter which is typed into the textbox.

Screenshot of the storybook demo:
custom-filterable-select

Current behaviour

Custom filtering and styling is not possible, because the FilterableSelectList component manipulates the child Option components as the user enters a text filter.

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Cypress automation tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in CodeSandbox/storybook
  • Add new Cypress test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Testing instructions

The following CodeSandbox is an example of the previous behaviour, where filtering breaks the option styling:
https://codesandbox.io/s/carbon-quickstart-j5pb2

You can see the new behaviour by looking at the version below:
https://codesandbox.io/p/sandbox/carbon-quickstart-typescript-forked-lr2pr9

@stripeyjumper stripeyjumper requested a review from a team as a code owner December 5, 2023 13:52
Copy link

codesandbox-ci bot commented Dec 5, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 11c9a1f:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart-typescript Configuration
carbon-quickstart PR

@@ -753,3 +753,65 @@ export const SelectionConfirmedStory = () => {
};

SelectionConfirmedStory.parameters = { chromatic: { disableSnapshot: true } };

export const CustomFilterAndOptionStyle = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Praise: this is a really good demo of what this feature can support 👏

nineteen88
nineteen88 previously approved these changes Jan 8, 2024
@nineteen88
Copy link
Contributor

Looks good, and solid reasoning for this proposal. Just the one non-blocking comment to resolve from Ed should you choose to do so then.

edleeks87
edleeks87 previously approved these changes Jan 9, 2024
@@ -125,6 +128,7 @@ export const FilterableSelect = React.forwardRef(
inputRef,
enableVirtualScroll,
virtualScrollOverscan,
disableFiltering = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I suggest this prop name is renamed to something like disableDefaultFiltering? I was struggling to understand the purpose of this, initially thinking filtering was disabled altogether and therefore what was the purpose of this change to Filterable Select at all if no filtering was then allowed until it was explained to me.

edleeks87
edleeks87 previously approved these changes Jan 18, 2024
Copy link
Contributor

@sianford sianford left a comment

Choose a reason for hiding this comment

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

Testing passed.

@stripeyjumper stripeyjumper force-pushed the filterable-select-disable-filtering branch from 63baadb to 3bd9903 Compare January 25, 2024 11:44
@stripeyjumper stripeyjumper force-pushed the filterable-select-disable-filtering branch from 3bd9903 to 11c9a1f Compare January 25, 2024 11:46
@stripeyjumper stripeyjumper changed the title feat(filterable-select): add disableFiltering prop feat(filterable-select): add disableDefaultFiltering prop Jan 25, 2024
@sianford sianford merged commit d294356 into Sage:master Jan 25, 2024
23 checks passed
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 125.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants