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] Dynamic colouring with last value #101583

Closed
dej611 opened this issue Jun 8, 2021 · 9 comments · Fixed by #101750
Closed

[Lens] Dynamic colouring with last value #101583

dej611 opened this issue Jun 8, 2021 · 9 comments · Fixed by #101750
Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@dej611
Copy link
Contributor

dej611 commented Jun 8, 2021

The current dynamic colouring feature for datatable is not correctly handling array values (using Last value operation).

Every cells gets assigned the first color stop bucket:

Screenshot 2021-06-08 at 14 38 05

I cannot see any "natural" solution here, other than pick an arbitrary route and make it clear to the user the actual logic for it.

Possible solutions (mentioned so far) are:

  • Color based on the first numeric value of the array
  • Do not enable the feature if an array is detected in the current table
  • Do not enable the feature for the specific cell with the array
  • something else...?

cc @ghudgins @flash1293 @markov00

@dej611 dej611 added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Jun 8, 2021
@elasticmachine
Copy link
Contributor

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

@ghudgins
Copy link

ghudgins commented Jun 8, 2021

to me the best solution (and what i'd expect as a gentle user) is your first and third bullet: if the value in the cell is a single number it would work and, alternatively, if the value isn't numeric (you tried to color by value the last value of a string / array) that nothing would happen. So in the above case color by value wouldn't do anything since there are multiple values per row. However, last value could work if the aggregation returns a single number. @MichaelMarcialis might want to weigh in here too since this is regarding user expectations / experience.

not sure how expensive of an ask that is....

@dej611
Copy link
Contributor Author

dej611 commented Jun 8, 2021

Do you think a warning message is required in such cases?

@markov00
Copy link
Member

markov00 commented Jun 8, 2021

@dej611 not an easy task actually, but there are probably solutions.

Do not enable the feature for the specific cell with the array

To me, this could be a valid option. In this case, I think we should do something to visually render better the fact that the cell is a multi-value/array cell. An icon, a different way of representing the values instead of the comma separator or something else can probably be useful to understand this situation better.

I have also another possible idea: why not expanding the rows for each field that contains an array? Something like a table denormalization? In this way each row actually represents a single value, we can correctly color-coding them, and we clearly represent a field array.
We could also combine this to the previously mentioned readability improvement: what if at first we have all the array fields collapsed (not color-coded), and we can expand them (a single or every array field in that column) with a click on the table cell?
We can also use this to introduce a tooltip on such cells that describe what and why this is happening, removing the warning message giving the user positive and actionable information.

@flash1293
Copy link
Contributor

I have also another possible idea: why not expanding the rows for each field that contains an array? Something like a table denormalization? In this way each row actually represents a single value, we can correctly color-coding them, and we clearly represent a field array.

That's a nice idea, @markov00 . I think we would need something like elastic/eui#4636 to do this, but I think it would a great follow-up.

@dej611
Copy link
Contributor Author

dej611 commented Jun 8, 2021

Created a PoC with some of the listed solutions : #101619

I have also another possible idea: why not expanding the rows for each field that contains an array? Something like a table denormalization? In this way each row actually represents a single value, we can correctly color-coding them, and we clearly represent a field array.

While this is nice I think it requires some upstream solution first. 😅

We could also combine this to the previously mentioned readability improvement: what if at first we have all the array fields collapsed (not color-coded), and we can expand them (a single or every array field in that column) with a click on the table cell?
We can also use this to introduce a tooltip on such cells that describe what and why this is happening, removing the warning message giving the user positive and actionable information.

I think this is some kind of doable right now (maybe via cellActions?). If interested I can add it to the PoC.

@ghudgins
Copy link

ghudgins commented Jun 8, 2021

after using the POC I think the skip is the best because we don't have to show an error (although we could). it's pretty clear why it's happening this way...hard to reason about the other color options...but maybe that's just me.

@flash1293
Copy link
Contributor

Agreed with @ghudgins , especially because in it's current state last value arrays are not preserving the order on ingestion which can get extra confusing: #99628

@dej611
Copy link
Contributor Author

dej611 commented Jun 9, 2021

Ok, I'll create a PR with the skip option then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants