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.

Sign up for GitHub

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

Jump to bottom

[Discover] Update Field statistics toggle in Discover & Advanced settings description to include docs #122344

Merged
merged 6 commits into from
Jan 12, 2022
Merged

[Discover] Update Field statistics toggle in Discover & Advanced settings description to include docs #122344

merged 6 commits into from
Jan 12, 2022

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Jan 5, 2022

Summary

Changes include:

  • Remove the beaker icon to the word Beta in the Discover toggle mode, as well as gives description to the the Advanced setting.

Screen Shot 2021-12-21 at 12 15 55

  • Update the description in the Advanced settings to be more explicit what the functionality is, as well as link to the new docs

Screen Shot 2022-01-05 at 09 11 53

@qn895 qn895 added Feature:Discover Discover Application :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Jan 5, 2022
@qn895 qn895 self-assigned this Jan 5, 2022
@qn895 qn895 marked this pull request as ready for review January 5, 2022 17:19
@qn895 qn895 requested a review from a team as a code owner January 5, 2022 17:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

peteharverson
peteharverson reviewed Jan 5, 2022
View reviewed changes
src/plugins/discover/server/ui_settings.ts Outdated
defaultMessage: `Enable {fieldStatisticsDocs} in Discover to explore the fields in your data. This functionality is in beta and is subject to change. `,
values: {
fieldStatisticsDocs:
`<a href="https://www.elastic.co/guide/en/kibana/master/show-field-statistics.html"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a doc links service that can be used for Discover settings? Hardcoded URLs, with the version hardcoded like master aren't normally used are they?

Copy link
Contributor

Choose a reason for hiding this comment

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

Has the docs page not been created yet? I just get a 404.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not merged yet, but we have to create this PR to get the screenshot for the Docs PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI The URL will still be broken until (1) the docs are backported to 7.17, which is "current" now, or (2) 8.0 GAs and becomes "current". I think that's okay for now, but if not you'd have to put a specific version in these URLs.

@qn895 qn895 requested a review from a team as a code owner January 5, 2022 18:08
@qn895
Copy link
Member Author

qn895 commented Jan 6, 2022

Is there a doc links service that can be used for Discover settings? Hardcoded URLs, with the version hardcoded like master aren't normally used are they?

Talked to Lisa and the docLinks service is indeed the recommended way moving forward, but it looks like the service is not available yet for server side/these UI settings. Here's the issue for it #95389

@qn895 qn895 removed the request for review from a team January 6, 2022 15:48
peteharverson
peteharverson approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

lcawl
lcawl approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@lcawl lcawl left a comment

Choose a reason for hiding this comment

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

One comment, but LGTM

@lcawl lcawl mentioned this pull request Jan 6, 2022
Closed
@qn895
Copy link
Member Author

qn895 commented Jan 10, 2022

@elasticmachine merge upstream

kertal
kertal approved these changes Jan 12, 2022
View reviewed changes
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.

LGTM, code review, didn't test

@qn895
Copy link
Member Author

qn895 commented Jan 12, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 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 330.7KB 330.7KB -18.0B

History

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

cc @qn895

@qn895 qn895 added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 12, 2022
@qn895 qn895 merged commit 0f2a047 into elastic:main Jan 12, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 12, 2022
@qn895 @kibanamachine
[Discover] Update Field statistics toggle in Discover & Advanced sett…
188a0c6 
…ings description to include docs (elastic#122344)

* Update beta badge & docs

* Add docs link to Discover, update to current

* Revert change to docLinks service

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 0f2a047)
@kibanamachine kibanamachine mentioned this pull request Jan 12, 2022
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 12, 2022
@kibanamachine @qn895
[Discover] Update Field statistics toggle in Discover & Advanced sett…
836c145 
…ings description to include docs (#122344) (#122808)

* Update beta badge & docs

* Add docs link to Discover, update to current

* Revert change to docLinks service

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 0f2a047)

Co-authored-by: Quynh Nguyen <43350163+qn895@users.noreply.github.com>
@qn895 qn895 deleted the ml-discover-beta-docs branch January 20, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcawl lcawl lcawl approved these changes

@peteharverson peteharverson peteharverson approved these changes

@kertal kertal kertal approved these changes

@gchaps gchaps Awaiting requested review from gchaps

Assignees

@qn895 qn895

Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application :ml release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v8.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@qn895 @elasticmachine @kibana-ci @kibanamachine @kertal @peteharverson @lcawl