-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 #135887
Comments
dear @elastic/kibana-design just wanted to know if it's fine we use the Dashboard style of showing "Unsaved changes" in Discover once we are able to do this |
@kertal I think it's ok to follow the same thing that Dashboard is doing including the confirmation modal when attempting to leave the page. |
thx @andreadelrio , let's first take care of the "Unsaved changes" badge. The confirmation modal could be done in a follow up, also needs much more changes. |
While this is probably not the best venue and should likely be discussed as a separate issue, I wanted to mention a thought I had here for the sake of visibility. It's always struck me as a odd to have this "Unsaved changes" badge in Dashboards that isn't in close proximity to the save action or the dashboard name. Instead, it's floating in an arbitrary location in the second Kibana header bar. The badge and its placement also doesn't scale down very elegantly at smaller viewport sizes. Given that we already enable/disable the save button when checking for a dirty state, perhaps we could consider replacing the current badge pattern with a simple dot icon next to the saved object name in the breadcrumbs to indicate a dirty state. This dot-next-to-name pattern is something that is used fairly consistently in a variety of software, and would allow the dirty state to be represented in a much more meaningful location, while also being less obtrusive and scale down better. Thoughts? |
I do agree there'S room for improvement. In case we change it, we should apply it also in Dashboard. I like the idea of moving the indicator to the left side, so it's better linke with the name of the saved search. One thing that comes to my mind, that it's maybe a bit to subtile compared to the badge. I'd like to add an additional design challenge. When you load a saved search, there's button displayed to reset it / undo changes. It would also be an opportunity to move that button up, because it's located where the histogram is, why : because it was "always like that", which is never a good answer. FYI @andreadelrio |
Bit of a sidenote but thought of this after reading Michael's suggestion. @kertal how do the |
Yes, good call, that's definitely something to consider, and a source for confusion, that's why it ain't an easy task for changes in this area |
@andreadelrio: Just now realizing I never responded to this! You raise a good point. I tried to account for this in my suggestion by having the unsaved changes |
@MichaelMarcialis @andreadelrio until we find a final solution for this, would it be ok to align to the dashboard solution, because it wouldn't be too much effort and already beneficial to have an indication for unsaved changes like this. In a follow up we could aim for a bigger change (which we should) |
…d search (#169548) - Resolves #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">  ### 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>
For iterating on the redesign of Discover, we need to change the UI for resetting the search, when a saved search is loaded
This link should be replaced by
This new badge can take advantage of hasChanged$ observable of the DiscoverSavedSearchContainer.
kibana/src/plugins/discover/public/application/main/services/discover_saved_search_container.ts
Line 69 in d974446
Originally the link is displayed when a saved search is loaded, no matter if it was modified, or not.
The text was updated successfully, but these errors were encountered: