-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[ES|QL] Supports chain controls #242909
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
base: main
Are you sure you want to change the base?
[ES|QL] Supports chain controls #242909
Conversation
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.
The change here is:
- if the supportsControl flag is enabled we display the "Create control" suggestion
- if variables exist we suggest them regardless of the supportsControl flag
| }) => { | ||
| const popoverId = useMemo(() => htmlIdGenerator()(), []); | ||
| const { componentApi, displaySettings } = useOptionsListContext(); | ||
| const { componentApi, displaySettings, customStrings } = useOptionsListContext(); |
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.
The invalid selection label is not making sense here as the value is valid (the chart works) but is not part of the updated list.
In order to introduce a new custom label I add a customStrings option in the context. It can be a property too. Let me know what you prefer.
|
Pinging @elastic/kibana-esql (Team:ESQL) |
|
Just adding a comment here for transparency. As discussed offline, we will be waiting until the Controls Anywhere feature lands before merging this work, since the conflicts here will be pretty nasty and that work takes priority 🙇 |
|
@Heenawter as long as you take care of the conflicts cc @teresaalvarezsoler |
|
Actually I am going to move the code on the ES|QL packages in another PR #243934 as it can be merged sooner and then you can take care of the rest. For this reason I am going to close this PR and move the issue to the presentation team. |
…3934) ## Summary Part of #238473 This PR is a necessary step before chaining controls. In the chaining controls we want to get variables suggestions but we do not want to get the "Create control" suggestion. This PR is accomplishing this with some small changes on how we treat the supportsControl prop in the editor. Note: I decided to move this away from the #242909 as the presentation team is working on a big refactoring so they will take care of the chaining controls. ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
…stic#243934) ## Summary Part of elastic#238473 This PR is a necessary step before chaining controls. In the chaining controls we want to get variables suggestions but we do not want to get the "Create control" suggestion. This PR is accomplishing this with some small changes on how we treat the supportsControl prop in the editor. Note: I decided to move this away from the elastic#242909 as the presentation team is working on a big refactoring so they will take care of the chaining controls. ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
|
We decided to move forward with this PR. I am moving it to draft to check that it still works ok and I will give it for review tmr! |
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Module Count
Async chunks
Page load bundle
History
|
Summary
Closes #238473
Adds support of chaining variable controls (ES|QL). Specifically:
For the above I had to make a small change in the editor. Now the editor can suggest variables if they are being provided but can only suggest the
Create controlonly if the supportsControl flag is enabled. For the above example we want the variables to be suggested but not the create controlChecklist