-
Notifications
You must be signed in to change notification settings - Fork 917
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: added a new API to allow adding additional editor controls #7936
feat: added a new API to allow adding additional editor controls #7936
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
@kavilla @ashwin-pc @abbyhu2000 Would love to see your comments :) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7936 +/- ##
==========================================
- Coverage 61.02% 61.01% -0.01%
==========================================
Files 3688 3688
Lines 87223 87226 +3
Branches 13422 13422
==========================================
- Hits 53224 53222 -2
- Misses 30772 30776 +4
- Partials 3227 3228 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -60,6 +62,7 @@ export interface QueryEditorProps { | |||
filterBar?: any; | |||
prepend?: React.ComponentProps<typeof EuiCompressedFieldText>['prepend']; | |||
savedQueryManagement?: any; | |||
additionalControls$?: BehaviorSubject<SearchBarControl[]>; |
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.
Why do we need this to be an observable?
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.
Might not be a hard requirement now, but I was thinking, it might make sense to make it an observable so that the controls can be added on the fly, similar to dynamic action triggers.
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.
There are 2 problems I see with this approach.
- Whats the usecase for such dynamic controls? Without a use I don't want to add more complexity to the query bar. It think it's more useful to pass the current app state to determine if the controls are needed rather than having an observable.
- Right now you can only add controls but not remove them. But this is an easy fix compared to the first one though
Signed-off-by: Yulong Ruan <[email protected]>
cd37b56
to
a416e6c
Compare
Description
Allow externals to add additional query editor controls
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration