-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiSelectable] export SiteWideRenderOptions and wire-up isPreFiltered
#4305
[EuiSelectable] export SiteWideRenderOptions and wire-up isPreFiltered
#4305
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_4305/ |
6dfb861
to
a0e2722
Compare
isPreFiltered
Preview documentation changes for this PR: https://eui.elastic.co/pr_4305/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at the code and LGTM!
But @chandlerprall or @thompsongl are probably more indicated to do the review.
@@ -288,6 +288,7 @@ export { | |||
EuiSelectableMessage, | |||
EuiSelectableSearch, | |||
EuiSelectableTemplateSitewide, | |||
euiSelectableTemplateSitewideRenderOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchaos I noticed that you named this euiSelectableTemplateSitewideRenderOptions
starting with a lower e
. Was this on purpose? Is it because is not a type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't a type and not a "proper" React component that can be used in JSX, but is a function which happens to return a React element. Because it's only a function, it begins with lowercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM; tested isPreFiltered functionality locally
Merging this as @myasonik is out today. |
Summary
Fixes #4277
This PR does two things:
euiSelectableTemplateSitewideRenderOptions
so that consumers can customize therenderOption
around it while still using the same option componentsearchValue
that gets passed in to control highlightingisPreFiltered
to be passed into the component and pass it along to the sorting function. (The sorting function already supported this flag but there was no way to use it.)Checklist
Also tested changes in Kibana confirming everything works.
[ ] Check against all themes for compatibility in both light and dark modes[ ] Checked in mobile[ ] Checked in Chrome, Safari, Edge, and Firefox[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples[ ] Checked for accessibility including keyboard-only and screenreader modes