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

[Lens] Better defaults for top values odering #97099

Merged
merged 7 commits into from
Apr 19, 2021

Conversation

flash1293
Copy link
Contributor

Fixes #87115

This PR changes the default rank behavior of the top values function to make it more likely to pick the desired output without explicit user interaction.

It introduces a new piece of state for a dimension, recording whether it's using alphabetical sorting because the user explicitly chose alphabetical or whether there was no other option

Behaviors:

  • If a sortable metric gets added and a top values dimension is currently using alphabetical sorting because there was no other option, it will pick up descending sorting by this new metric
  • If a top values dimension is using alphabetical sorting because of an explicit user interaction, it won't be affected by changing metric dimensions
  • If the metric a top values dimension is ranked by gets removed and there is another metric to rank by, it will switch over to that one
  • If the metric a top values dimension is ranked by gets removed and there is no other metric to rank by, it will switch over to ascending alphabetical ordering

@flash1293 flash1293 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 14, 2021
@flash1293 flash1293 marked this pull request as ready for review April 15, 2021 11:26
@flash1293 flash1293 requested a review from a team April 15, 2021 11:26
@elasticmachine
Copy link
Contributor

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

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.

Nice, this works exactly how I expected it to. My only comment is about whether we want to store this in the persisted state vs local state only: if you open a saved visualization that uses a fallback it will have the same state.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@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 943.8KB 944.5KB +732.0B

History

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

@flash1293
Copy link
Contributor Author

My only comment is about whether we want to store this in the persisted state vs local state only: if you open a saved visualization that uses a fallback it will have the same state.

I see the point - I thought it would make sense to persist the flag as well. Let's see whether people get annoyed with it.

@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 Apr 19, 2021
Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
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:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Warn or prevent users from sorting the terms aggregation by count in ascending order
4 participants