Skip to content
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] Enable description for saved search modal #114257

Merged
merged 12 commits into from
Oct 19, 2021

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Oct 7, 2021

Summary

Closes #110667

This PR enables description for saved search modal.

Top nav buttons

EED91AFA-AAA2-4D9F-9FBD-108261A6035A_4_5005_c

Saved search modal

95F7FDBD-DC7B-4477-A9C3-4C01D424EA11

@dimaanj dimaanj added Feature:Discover Discover Application v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Oct 7, 2021
@dimaanj dimaanj self-assigned this Oct 7, 2021
@dimaanj dimaanj requested a review from a team as a code owner October 7, 2021 11:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@dimaanj dimaanj changed the title [Discover] Enable description fields for saved search modal [Discover] Enable description for saved search modal Oct 7, 2021
@kertal
Copy link
Member

kertal commented Oct 11, 2021

@elasticmachine merge upstream

'Save your Discover search so you can use it in visualizations and dashboards',
})}
showDescription={false}
showDescription={true}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's something odd here in the underlying component, so you could just display a general description on top of the form, or enable the description form element. I wonder why?? since having a description on top like it was before can be helpful. dear @timroes, do you know why it was built that way?

Code LGTM, else, just wanted to clarify this, since we remove stuff here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there was any logical reason if this is an explicit OR for those two. I think we can address that, if we would like to show both. But since none of the other save dialogs have a description in it, I'd actually be fine also removing it in Discover. I think it makes more sense having an aligned experience here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with that aligning stuff. But one other thought about that. If we enable the description here, we should also show it somewhere? Else we allow user input of something that is just visible on saving.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it should be visible when inspecting saved object in stack management.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe -untested - it will also automatically already show on dashboard for that panel. But I totally agree it would be better if we also show it in Discover. Though I am not sure if it makes sense to address that without a general change of our loading/listing page, which we revently discussed, i.e. not sure if it's worth spending the time now building the description into the open panel as it is today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed this offline, just for posterity: we would like to add the description, and make sure that the description is shown on dashboards the same as for the visualize_embeddable. This does not seem to work out of the box, so we need to check how to get it showing up on panels :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

…e-description-for-ss-modal

# Conflicts:
#	src/plugins/discover/public/application/apps/main/components/top_nav/on_save_search.tsx
@dimaanj dimaanj requested review from timroes and kertal October 13, 2021 13:41
@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 15, 2021

@elasticmachine merge upstream

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, tested a-la-carte in Safari / Chrome / Firefox, OSX, works as expected
Nice change aligning it to Dashboard
Bildschirmfoto 2021-10-15 um 16 37 52
Description is displayed on the dashboard

Bildschirmfoto 2021-10-15 um 16 38 27
.

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 18, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Oct 18, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Oct 18, 2021

@elasticmachine merge upstream

@dimaanj dimaanj added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 18, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 328.9KB 328.8KB -32.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dmitriynj

@dimaanj dimaanj merged commit 4d2f769 into elastic:master Oct 19, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 19, 2021
* [Discover] enable description for saved search

* [Discover] remove i18n translations for removed description

* [Discover] apply Tim's suggestion

* [Discover] update snapshot

* [Discover] reorder top nav buttons in tests

* [Description] fix description save action

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 19, 2021
* [Discover] enable description for saved search

* [Discover] remove i18n translations for removed description

* [Discover] apply Tim's suggestion

* [Discover] update snapshot

* [Discover] reorder top nav buttons in tests

* [Description] fix description save action

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Dmitry Tomashevich <39378793+Dmitriynj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Align Save modal with other apps
5 participants