[discover] Query editor UI changes #7866
Conversation
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
abbyhu2000
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli and
joshuali925
as code owners
❌ 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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7866 +/- ##
==========================================
- Coverage 64.17% 64.16% -0.01%
==========================================
Files 3659 3659
Lines 80796 80797 +1
Branches 12866 12869 +3
==========================================
- Hits 51848 51843 -5
- Misses 25769 25772 +3
- Partials 3179 3182 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return ( | ||
| <div> | ||
| <EuiFlyout | ||
| className="observability-flyout" |
| @@ -0,0 +1,4 @@ | |||
| .recentQuery__table { | |||
| padding: $euiSizeXS; | |||
| width: 1320px; | |||
There was a problem hiding this comment.
Why do we need this fixed width? What if i look at it on a smaller screen?
| return ( | ||
| <div> | ||
| <EuiFlyout | ||
| className="observability-flyout" |
There was a problem hiding this comment.
nit: The class name should be updated
| setIsLanguageReferenceOpen(false); | ||
| }; | ||
|
|
||
| const osdDQLDocs = 'https://opensearch.org/docs/2.16/dashboards/dql)'; |
There was a problem hiding this comment.
Can we get the version from package.json?
| defaultMessage: `Language Reference`, | ||
| }), | ||
| run: async (anchorElement) => { | ||
| if (props.queryLanguage === 'PPL' || props.queryLanguage === 'SQL') { |
There was a problem hiding this comment.
To keep the code cleaner, cant we register these when we register the PPL and SQL language using the additional controls property?
There was a problem hiding this comment.
I'd move all these langugae docs to the query enhancements plugin
| @@ -112,9 +112,9 @@ export class QueryStringManager { | |||
| }; | |||
|
|
|||
| // Todo: update this function to use the Query object when it is udpated, Query object should include time range and dataset | |||
There was a problem hiding this comment.
Nit: We should remove this TODO
| }; | ||
|
|
||
| const queryControls = | ||
| props.queryLanguage === 'PPL' || props.queryLanguage === 'SQL' |
There was a problem hiding this comment.
Lets not check explicitly for the PPL/SQL stuff here. The expand should be associated with the editor itself. If its the DQL editor, it should have this but if its the Default editor, then it should have this control alongside the other controls
There was a problem hiding this comment.
Approving with agreement of fast follows.
Avoid hardcode ref to language in data plugin. Fast follow should consider modifying the contract of the language config to have more features. And if the language doesn't include it then dont render it. An example I would reference is Joshua's query editor extension. You can just render an array of actions.
For example you could pass in a button.
For the expand and collapsed I think it's weird the shifting of other buttons and that it's in the middle of the other actions even though it manipulates something not to the left of it. Like if I was tabbing through I could be typing and then I have to click tab twice otherwise the first button opens an overlay
Ive seen other applications include the expand and collapsed within the actual editor. Which makes sense for me because then I won't have to misclick on the wrong button potentially and open an overlay if I wanted to go from the query editor to expand it
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-7866-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 333957b1ad7732774504dab53467e9e2d09c0c43
# Push it to GitHub
git push --set-upstream origin backport/backport-7866-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.xThen, create a pull request where the |
Description
Issues Resolved
Screenshot
filter button:
Screen.Recording.2024-08-27.at.12.26.54.PM.mov
query controls:
https://github.com/user-attachments/assets/18e1450f-4b3d-4dfb-9e8c-14495235cc6c
recent query:
Screen.Recording.2024-08-27.at.12.25.03.PM.mov
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration