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
[Discover] Show "unsaved changes" label when in unsaved state of saved search #169548
[Discover] Show "unsaved changes" label when in unsaved state of saved search #169548
Conversation
… into 135887-discover-unsaved-changes
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.
log_explorer
changes LGTM
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.
Code changes look great (thanks for using RTL for new tests 🙌), and it's working well overall! I think there are some improvements we could make as followups, like maybe directly saving the search when clicking the "Save" button, but we don't need to add more to this PR and I think it's working great as an initial implementation.
The only issue I encountered other than a stuck popover was that I somehow got the search into a state where the changes had to be reverted twice in order for the badge to disappear. I couldn't reproduce it consistently, but it seemed to happen after I made changes to most of the state and then switched data views:
revert.mp4
...ges/kbn-unsaved-changes-badge/src/components/unsaved_changes_badge/unsaved_changes_badge.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/services/discover_saved_search_container.ts
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/utils/update_saved_search.ts
Outdated
Show resolved
Hide resolved
I made some updates to address it. Could you please set |
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.
With the latest changes I can no longer reproduce the issue I was seeing, and I didn't encounter any other issues while testing. Great job on this, and it's a significant improvement over the reset button! LGTM 👍
@@ -246,26 +247,65 @@ export function isEqualSavedSearch(savedSearchPrev: SavedSearch, savedSearchNext | |||
...Object.keys(prevSavedSearch), |
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.
GitHub updated their comment component and apparently broke multiline suggestions so I can't add it as a suggestion, but could we type keys
so that we can drop all the @ts-expect-error
comments below?
const keys = new Set([
...Object.keys(prevSavedSearch),
...Object.keys(nextSavedSearchWithoutSearchSource),
] as Array<keyof Omit<SavedSearch, 'searchSource'>>);
const savedSearchDiff = [...keys].filter((key: string) => { | ||
|
||
// at least one change in saved search attributes | ||
const hasChangesInSavedSearch = [...keys].some((key: string) => { |
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.
const hasChangesInSavedSearch = [...keys].some((key: string) => { | |
const hasChangesInSavedSearch = [...keys].some((key) => { |
No need to type as string
if we type keys
.
@elasticmachine merge upstream |
Merge queue setting changed
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @jughosta |
…d search (elastic#169548) - Resolves elastic#135887 ## Summary This PR adds "Unsaved changes" badge to Discover for modified saved searches. It also removes "Reset search" button from the histogram area. Code for the badge is added to a new package `@kbn/unsaved-changes-badge`. <img width="600" alt="Screenshot 2023-10-23 at 18 05 34" src="https://github.com/elastic/kibana/assets/1415710/ad200a28-79e1-4cc5-8e28-6352d4b85322"> ![Oct-23-2023 18-06-39](https://github.com/elastic/kibana/assets/1415710/cacf4ff2-525c-4759-aba9-34ce75089ddd) ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [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 - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Summary
This PR adds "Unsaved changes" badge to Discover for modified saved searches. It also removes "Reset search" button from the histogram area. Code for the badge is added to a new package
@kbn/unsaved-changes-badge
.Checklist