-
Notifications
You must be signed in to change notification settings - Fork 884
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
Saved queries new UI #8469
base: main
Are you sure you want to change the base?
Saved queries new UI #8469
Conversation
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/save_query_flyout.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/save_query_flyout.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/saved_query_card.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8469 +/- ##
==========================================
- Coverage 60.84% 60.74% -0.11%
==========================================
Files 3793 3798 +5
Lines 90486 90646 +160
Branches 14212 14254 +42
==========================================
+ Hits 55057 55061 +4
- Misses 31939 32089 +150
- Partials 3490 3496 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
e829b40
to
ed0cef2
Compare
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
4d88836
to
a7cf66c
Compare
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
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.
Didnt get a chance to review the whole thing yet. But a quick question on saved query templates. I see you added the tab, but I couldnt find a way in the UI to create them. Also if our plan is to include them dynamically, can we hide the tab if there arent any present? Having an empty tab there for most users is bad UX
src/plugins/data/public/query/saved_query/saved_query_service.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Will follow up with another PR to update the UI with either:
Another angle is that now since we will always have some sample queries that we show where there are no results, we could package them as templates as well. Will work on the best path forward and create a follow up PR |
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
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 tests failing related to saved queries.
Also, we've added a lot of code without tests - how are we tracking following up with those tests?
src/plugins/data/public/query/saved_query/saved_query_service.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Outdated
Show resolved
Hide resolved
}, [savedQueries, searchQuery, activePage, itemsPerPage]); | ||
|
||
const onChange: EuiSearchBarProps['onChange'] = ({ query, error }) => { | ||
if (!error) { |
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.
what if there is an error?
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 will show no results which is fine since user will simply need to change the search query
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/open_saved_query_flyout.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/saved_query_card.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/saved_query_card.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/saved_query_flyouts/saved_query_card.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
Let me add a task on myself to add new tests |
Signed-off-by: Amardeepsingh Siglani <amardeep7194@gmail.com>
d437eee
to
39f92ac
Compare
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.
I did not have time to pull this down to validate, and looks like there are still some more cases of text to be localized, but approving assuming that and unit tests will be followed up with.
{ | ||
type: 'field_value_selection', | ||
field: 'datasetType', | ||
name: 'Data 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.
is this label presented to customers as should it be localized?
Description
This PR updates the UI for saved queries to use Flyouts which give more surface to search and categorize the queries as well as introduce more options when saving them.
Screenshot
Testing the changes
Tested change with local setup.
Changelog
Check List
yarn test:jest
yarn test:jest_integration