-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Input Controls] Options List Embeddable, Factory & Frame #106877
[Input Controls] Options List Embeddable, Factory & Frame #106877
Conversation
…ibana into controls/initialOptionsListComponent
…ialOptionsListComponent
…ialOptionsListComponent
Pinging @elastic/kibana-presentation (Team:Presentation) |
@elasticmachine merge upstream |
[Input Controls] Using EuiFilterButton
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.
Andrea is out this week. Is it safe to assume we can merge this, from a design perspective, and come back to additional tweaks after this is merged? From what I understood, this was big step along an iterative journey. (i.e. I'm considering just hitting approve for the SCSS changes)
@elasticmachine merge upstream |
@ryankeairns, I was also out last week, but yes I believe now that the design PR has been merged in, this should be safe to merge from a design perspective! I will ask @andreadelrio to review & triple-check that though. |
@ThomThomson This is looking good to me. I have a related PR up on the EUI repo fixing the issue of hiding the count when no selections have been made. |
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.
Looks good overall! Few small questions but the storybook demo is very neat! My one other thought is about where this lives. Is its home presentation util plugin permanently or is this where the POC is kinda being worked on and built out?
setAvailableOptions(newAvailableOptions); | ||
|
||
if (toggleOff) selectedOptions.current.delete(item.label); | ||
else selectedOptions.current.add(item.label); |
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.
nit: the two line if-else here is a little hard to read but that might just be me
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.
Yeah, some braces here might help readability!
})} | ||
> | ||
<EuiPopover | ||
id="popoverExampleMultiSelect" |
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.
Is this id correct? I can't find a reference to it in this PR
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.
Looks like that's a leftover from copying from the EUI examples - will remove!
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Detection Engine API Integration Tests.x-pack/test/detection_engine_api_integration/security_and_spaces/tests/exception_operators_data_types/text·ts.detection engine api security and spaces enabled Detection exceptions data types and operators Rule exception operators for data type text "is not" operator will return 0 results if it cannot find what it is excludingStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Looks great and works as expected!
Note to future selves:
- add ability to clear options
- add ability to view only selected options
- add option to limit input width (i.e. so 1 input doesn't go full width)
…6877) Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com> Co-authored-by: andreadelrio <delrio.andre@gmail.com>
…108101) Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com> Co-authored-by: andreadelrio <delrio.andre@gmail.com> Co-authored-by: Clint Andrew Hall <clint@clintandrewhall.com> Co-authored-by: andreadelrio <delrio.andre@gmail.com>
Summary
This PR is a follow-up to #103512, which is intended to be merged into master.
This re-creates the options list component as an embeddable, with a couple notable improvements over the initial design
fetchData
method, which is passed in. In storybook this is a stub, and it can be replaced with a real implementation inside of Kibana.For maintainers