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

[Lens] Improves ranking feature in Top values #90749

Merged
merged 7 commits into from
Feb 15, 2021
Merged

[Lens] Improves ranking feature in Top values #90749

merged 7 commits into from
Feb 15, 2021

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Feb 9, 2021

Summary

Part of #81780

This PR clarifies some ambiguities for the ranking feature in the Top values operation in Lens.
The feature has been now renamed into Order -> Rank, and tooltips have been added to explain further the concepts.

Screenshot 2021-02-09 at 11 57 43

Screenshot 2021-02-09 at 11 57 48

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.12.0 labels Feb 9, 2021
@dej611 dej611 requested a review from KOTungseth February 9, 2021 11:09
@dej611 dej611 mentioned this pull request Feb 9, 2021
Open
@dej611
Copy link
Contributor Author

dej611 commented Feb 9, 2021

@elasticmachine merge upstream

@dej611
Copy link
Contributor Author

dej611 commented Feb 10, 2021

@elasticmachine merge upstream

@dej611 dej611 marked this pull request as ready for review February 10, 2021 14:46
@dej611 dej611 requested a review from a team February 10, 2021 14:46
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293
flash1293 reviewed Feb 10, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks almost good to me. Code review only

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx Outdated
<EuiIconTip
color="subdued"
content={i18n.translate('xpack.lens.indexPattern.terms.orderByHelp', {
defaultMessage: `It controls the ranking system used to retrieve the Top values.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

@KOTungseth Should "Top values" be capitalized here? I'm always hazy on these rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

Specifies the function that retrieves the top values.

Since we are referring to the top values in the data set instead of the UI setting, top values can be lowercase.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx
@@ -378,7 +411,7 @@ export const termsOperation: OperationDefinition<TermsIndexPatternColumn, 'field
)
}
aria-label={i18n.translate('xpack.lens.indexPattern.terms.orderBy', {
defaultMessage: 'Order by',
Copy link
Contributor

Choose a reason for hiding this comment

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

As you changed the default messages, please delete the translations in chinese and japanese i18n files as well.

KOTungseth
KOTungseth reviewed Feb 10, 2021
View reviewed changes
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx Outdated
<EuiIconTip
color="subdued"
content={i18n.translate('xpack.lens.indexPattern.terms.orderByHelp', {
defaultMessage: `It controls the ranking system used to retrieve the Top values.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

Specifies the function that retrieves the top values.

Since we are referring to the top values in the data set instead of the UI setting, top values can be lowercase.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx Outdated
<EuiIconTip
color="subdued"
content={i18n.translate('xpack.lens.indexPattern.terms.orderDirectionHelp', {
defaultMessage: `It controls the ranking direction used to retrieve the Top values.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

Specifies the ranking order of the top values.

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx
@@ -378,7 +411,7 @@ export const termsOperation: OperationDefinition<TermsIndexPatternColumn, 'field
)
}
aria-label={i18n.translate('xpack.lens.indexPattern.terms.orderBy', {
defaultMessage: 'Order by',
defaultMessage: 'Rank by',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a ranking system? Or an order system?

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 feel it's a tricky question :D .
I'd say it is a ranking system here to differentiate from order which sounds more as a postprocessing task in this context.

flash1293
flash1293 reviewed Feb 11, 2021
View reviewed changes
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx Outdated
<EuiIconTip
color="subdued"
content={i18n.translate('xpack.lens.indexPattern.terms.orderByHelp', {
defaultMessage: `Specifies the function that retrieves the top values.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading it in the UI, I'm not a fan of this wording. The function is not retrieving the top values - it's the dimension they are ranked by. The dropdown is not listing the functions from the function list on top, it's listing the metric dimensions of the current chart (plus alphabetical). Specifies the dimension the top values are ranked by maybe?

flash1293
flash1293 approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM

wylieconlon
wylieconlon approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

I haven't run this code, but the text changes LGTM. I'm not sure this needs a release note, what do you think?

@dej611
Copy link
Contributor Author

dej611 commented Feb 11, 2021

Maybe with only labels changed I would say to skip them for sure.
I based the label choice on other PRs with in-product tooltip/popovers explanation, as I see them as an improvement on the user experience.
But no strong argument to change it over to be honest, other than conformity with them. :)

@dej611
Copy link
Contributor Author

dej611 commented Feb 15, 2021

@elasticmachine merge upstream

@dej611 dej611 added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Feb 15, 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
lens 892.3KB 893.3KB +982.0B

History

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

@dej611 dej611 merged commit 75a7f78 into elastic:master Feb 15, 2021
@dej611 dej611 deleted the fix/order-by-top-values-lens branch February 15, 2021 11:25
@dej611 dej611 mentioned this pull request Feb 15, 2021
Merged
dej611 added a commit to dej611/kibana that referenced this pull request Feb 15, 2021
@dej611 @kibanamachine
[Lens] Improves ranking feature in Top values (elastic#90749)
ff08604 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 15, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/update-co…
28ed48d 
…ndition-for-hiding-recommded-allocation

* 'master' of github.com:elastic/kibana:
  [Discover] Fix toggling multi fields from doc view table (elastic#91121)
  [ML] Data Frame Analytics: ROC Curve Chart (elastic#89991)
  skip flaky suite (elastic#86948)
  skip flaky suite (elastic#91191)
  Fix date histogram time zone for rollup index (elastic#90632)
  [Search Source] Fix retrieval of unmapped fields; Add field filters (elastic#89837)
  [Logs UI] Use useMlHref hook for ML links (elastic#90935)
  Fix values of `products.min_price` field in Kibana sample ecommerce data set (elastic#90428)
  [APM] Darker shade for Error group details labels (elastic#91349)
  [Lens] Adjust new copy for 7.12 (elastic#90413)
  [ML] Unskip test. Fix modelMemoryLimit value. (elastic#91280)
  [Lens] Fix empty display name issue in XY chart (elastic#91132)
  [Lens] Improves error messages when in Dashboard (elastic#90668)
  [Lens] Keyboard-selected items follow user traversal of drop zones (elastic#90546)
  [Lens] Improves ranking feature in Top values (elastic#90749)
  [ILM] Rollover min age tooltip and copy fixes (elastic#91110)

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts
dej611 added a commit that referenced this pull request Feb 15, 2021
@dej611 @kibanamachine
[7.x] [Lens] Improves ranking feature in Top values (#90749) (#91385)
48a22ec 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KOTungseth KOTungseth KOTungseth left review comments

@mbondyra mbondyra mbondyra approved these changes

@flash1293 flash1293 flash1293 approved these changes

@wylieconlon wylieconlon wylieconlon approved these changes

Assignees
No one assigned
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dej611 @elasticmachine @kibanamachine @wylieconlon @flash1293 @mbondyra @KOTungseth