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] Other bucket defaults to false for top values greater than equal 1000 #167141

Merged
merged 14 commits into from
Oct 2, 2023

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Sep 25, 2023

Summary

Closes #162456

It switches off the other bucket switch if the user updates the terms to a number greater than equal 1000.

In the beginning I had made it simpler so the switch would change to false if size >= 1000 and to true if size < 1000. But I am not sure I like this experience so I decided to do something else:

  • The other bucket defaults to true if size < 1000
  • If the user changes to a value >=1000 then it changes to false (The user can always enable it)
  • If the user changes this to a value < 1000 it doesn't change back to true. The user must change it manually.

Let me know if you agree that this experience makes more sense.

image

Checklist

@stratoula stratoula changed the title [Lens] Other bucket defaults to false for top values greater than equ… [Lens] Other bucket defaults to false for top values greater than equal 1000 Sep 25, 2023
@stratoula stratoula added release_note:enhancement Feature:Lens backport:skip This commit does not require backporting v8.11.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 26, 2023
@stratoula stratoula marked this pull request as ready for review September 26, 2023 14:03
@stratoula stratoula requested a review from a team as a code owner September 26, 2023 14:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@dej611
Copy link
Contributor

dej611 commented Sep 26, 2023

I would expect to be notified, as a user, if as side effect of my input "automagically" switches a setting somewhere.
Recently we've been using info toasts to notify users when:

  • using specific operations when random sampling is enabled
  • using decimals in percentile/percentile rank when the column is used as ranking in Top values

Given that the Show Others options is also hidden by default behind the Advanced "curtains" I would propose to consider also in this case either the toast route, or alternative routes to notify users.

@stratoula
Copy link
Contributor Author

@dej611 honestly i wanted to add a toast and I forgot it. I will update tmr :)

@stratoula
Copy link
Contributor Author

@gchaps @amyjtechwriter can I have some help on the toast notification? The current state is:

image

This notification appears when the user changes the terms size from a value below 1000 to a value above. When this happens we set the Group remaining values as "Other" switch (which is on the advanced accordion) to false.

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested again and it works fine 👍

I have only a personal opinion about the behaviour when, within the same editing session, the user changes from 100 to 1000, then restores back to 100.
If the Group remaining values as "Others" was enabled at 100 initially, I would expect the same state to be restored at the end. If the setting was disabled I would expect instead it to remain so.
Not a blocker for this PR, but the current behaviour of not restoring the initial state itches me a bit. 😅
group_as_others

@stratoula
Copy link
Contributor Author

stratoula commented Sep 28, 2023

Thanx Marco, I am waiting for Gail's text proposal so I am not going to merge it without it.

I appreciate your review. Personally I think that at this case it is weird to enable it if the user types again the same number they had. This is an edge case and even if it happens we have already informed them with a notification that this switch is off. If we did what you propose in my mind it would require another notification because I don't think that it is expected that the switch will turn on again. We don't enable it for other cases so it is more confusing to enable it for this particular case. This complicates things in the UI without any good reason imho. (We can always change it if any user complains or finds it confusing)

@stratoula
Copy link
Contributor Author

stratoula commented Oct 2, 2023

With the new texts
image

@stratoula stratoula enabled auto-merge (squash) October 2, 2023 16:49
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

Text LGTM

@stratoula stratoula merged commit 93f9213 into elastic:main Oct 2, 2023
@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
lens 1.4MB 1.4MB +911.0B

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.11.0
Projects
None yet
6 participants