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: Add optionMatcher prop to EuiSelectable and EuiComboBox components #7709

Merged
merged 8 commits into from
Apr 29, 2024

Conversation

tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Apr 24, 2024

Summary

This PR resolves #2199 by allowing passing a custom matcher function to EuiComboBox and EuiSelectable using the new optional optionMatcher prop.

I kept the tests the same, just moved code around to extract the specific filtering logic while keeping component internals intact and not overridable (like isPreFiltered logic), so consumers don't have to reimplement them.

QA

The default option matcher should behave exactly as before the prop was introduced.

  • Compare option filtering of EuiComboBox on PR env and production and confirm there are no behavior changes
  • Compare option filtering of EuiSelectable on PR env and production and confirm there are no behavior changes
  • Confirm the Custom option matcher example uses startsWith matcher and only labels starting with search value are being matched

General checklist

  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

@tkajtoch tkajtoch self-assigned this Apr 24, 2024
* Partial string equality option matcher for EuiComboBox.
* It matches all options with labels including the searched string.
*/
export const createPartialStringEqualityOptionMatcher = <
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a factory function to keep TOption generic instead of defaulting to string/unknown or the underlying T type that defaults to string | number | string[] | undefined. The value property of type TObject is not used in this function so it doesn't really make a difference here

@tkajtoch tkajtoch marked this pull request as ready for review April 24, 2024 15:46
@tkajtoch tkajtoch requested a review from a team as a code owner April 24, 2024 15:46
Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

I tested it with wildcard matcher we use in Kibana and works great! Thank you!

@@ -98,3 +100,45 @@ export const Playground: Story = {
);
},
};

export const CustomMatcher: Story = {
render: function Render({ singleSelection, onCreateOption, ...args }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🗒️ A general note, mainly for myself here:
Whenever we align on the code-snippet API in this PR and merge it, this story needs to be updated as it's a case of "additional composition wrapper" (it will likely need the parameters.codeSnippet.resolveChildren: true as otherwise this story snippet would be <Render anyStoryArgsHere />

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the case for many of our stories, though, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some for sure. There are different cases I've seen so far. It depends on what should be output per story.
E.g. for cases like stateful wrappers that return just the actual story component it's different than a wrapper that returns the story component as nested child with other composition elements.

For stateful wrappers or related wrappers (Where a parent-subcomponent structure is required and can be determined based on naming it's already done automatically)
For anything else it might need adjustments.
I'm currently checking the newly added stories and there are some new cases as well that I did not consider yet (render functions 🙈)

Copy link
Contributor

@mgadewoll mgadewoll 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 and LGTM!

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @tkajtoch

@tkajtoch tkajtoch merged commit 1e83176 into elastic:main Apr 29, 2024
7 checks passed
cee-chen added a commit to elastic/kibana that referenced this pull request May 3, 2024
`v94.1.0-backport.0` ⏩ `v94.2.1-backport.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

##
[`v94.2.1-backport.0`](https://github.com/elastic/eui/releases/v94.2.1-backport.0)

**This is a backport release only intended for use by Kibana.**

- Reverted the `EuiFlexGroup`/`EuiFlexItem` `component` prop feature due
to Kibana typing issues

## [`v94.2.1`](https://github.com/elastic/eui/releases/v94.2.1)

**Bug fixes**

- Fixed an `EuiTabbedContent` edge case bug that occurred when updated
with a completely different set of `tabs`
([#7713](elastic/eui#7713))
- Fixed the `@storybook/test` dependency to be listed in
`devDependencies` and not `dependencies`
([#7719](elastic/eui#7719))

## [`v94.2.0`](https://github.com/elastic/eui/releases/v94.2.0)

- Updated `getDefaultEuiMarkdownPlugins()` to allow excluding the
following plugins in addition to `tooltip`:
([#7676](elastic/eui#7676))
  - `checkbox`
  - `linkValidator`
  - `lineBreaks`
  - `emoji`
- Updated `EuiSelectable`'s `isPreFiltered` prop to allow passing a
configuration object, which allows disabling search highlighting in
addition to search filtering
([#7683](elastic/eui#7683))
- Updated `EuiFlexGroup` and `EuiFlexItem` prop types to support passing
any valid React component type to the `component` prop and ensure proper
type checking of the extra props forwarded to the `component`.
([#7688](elastic/eui#7688))
- Updated `EuiSearchBar` to allow the `@` special character in query
string searches ([#7702](elastic/eui#7702))
- Added a new, optional `optionMatcher` prop to `EuiSelectable` and
`EuiComboBox` allowing passing a custom option matcher function to these
components and controlling option filtering for given search string
([#7709](elastic/eui#7709))

**Bug fixes**

- Fixed an `EuiPageTemplate` bug where prop updates would not cascade
down to child sections
([#7648](elastic/eui#7648))
- To cascade props down to the sidebar, `EuiPageTemplate` now explicitly
requires using the `EuiPageTemplate.Sidebar` rather than
`EuiPageSidebar`
- Fixed `EuiFieldNumber`'s typing to accept an icon configuration shape
([#7666](elastic/eui#7666))
- Fixed `EuiFieldText` and `EuiFieldNumber` to render the correct
paddings for icon shapes set to `side: 'right'`
([#7666](elastic/eui#7666))
- Fixed `EuiFieldText` and `EuiFieldNumber` to fully ignore
`icon`/`prepend`/`append` when `controlOnly` is set to true
([#7666](elastic/eui#7666))
- Fixed `EuiColorPicker`'s input not setting the correct right padding
for the number of icons displayed
([#7666](elastic/eui#7666))
- Visual fixes for `EuiRange`s with `showInput`:
([#7678](elastic/eui#7678))
  - Longer `append`/`prepend` labels no longer cause a background bug
  - Inputs can no longer overwhelm the actual range in width
- Fixed a visual text alignment regression in `EuiTableRowCell`s with
the `row` header scope
([#7681](elastic/eui#7681))
- Fixed `toolTipProps` type on `EuiSuperUpdateButton` to use
`Partial<EuiToolTipProps>`
([#7692](elastic/eui#7692))
- Fixes missing prop type for `popperProps` on `EuiDatePicker`
([#7694](elastic/eui#7694))
- Fixed a focus bug with `EuiDataGrid`s with `leadingControlColumns`
when moving columns to the left/right
([#7701](elastic/eui#7701))
([#7698](elastic/eui#7698))
- Fixed `EuiSuperDatePicker` to validate date string with respect of
locale on `EuiAbsoluteTab`.
([#7705](elastic/eui#7705))
- Fixed a visual bug with `EuiSuperDatePicker`'s absolute tab on small
mobile screens ([#7708](elastic/eui#7708))
- Fixed i18n of empty and loading state messages for the
`FieldValueSelectionFilter` component
([#7718](elastic/eui#7718))

**Dependency updates**

- Updated `@hello-pangea/dnd` to v16.6.0
([#7599](elastic/eui#7599))
- Updated `remark-rehype` to v8.1.0
([#7601](elastic/eui#7601))

**Accessibility**

- Improved `EuiBasicTable` and `EuiInMemoryTable`'s selection checkboxes
to have unique aria-labels per row
([#7672](elastic/eui#7672))
- Added `aria-valuetext` attributes to `EuiRange`s with tick labels for
improved screen reader UX
([#7675](elastic/eui#7675))
- Updated `EuiAccordion` to keep focus on accordion trigger instead of
moving to content on click/keypress
([#7696](elastic/eui#7696))
- Added `aria-disabled` attribute to `EuiHorizontalSteps` when status is
"disabled" ([#7699](elastic/eui#7699))

---------

Co-authored-by: Tomasz Kajtoch <tomasz.kajtoch@elastic.co>
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request May 3, 2024
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiComboBox] Custom matcher requested
5 participants