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][Table] Add color by terms with color mappings #189895

Merged
merged 50 commits into from
Aug 28, 2024

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Aug 5, 2024

Summary

Adds support for coloring table cells by terms with color mappings assignments. Supported for both Rows and Metric dimensions.

Zight.Recording.2024-08-05.at.07.46.55.AM.mp4

closes #179322

Checklist

@nickofthyme nickofthyme added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens release_note:feature Makes this part of the condensed release notes vis:table labels Aug 5, 2024
Copy link
Contributor Author

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

@teresaalvarezsoler & @MichaelMarcialis - Mostly complete POC still tweaking and updating/adding tests.

See a-la-carte deployment here. You might get a 502 gateway error at first or need to bypass the security but it eventually worked for me.

@teresaalvarezsoler
Copy link

Hey @nickofthyme, thanks for creating the deployment, it looks great! I couldn't find any issues. One question, the fact that we apply the old color mappings by default, is it because we don't offer the new color mappings for numbers? (I think that's fine, just clarifying the reasoning behind)

Copy link

@teresaalvarezsoler teresaalvarezsoler left a comment

Choose a reason for hiding this comment

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

Everything looks good to me from the user perspective.

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.

Had a first review pass, not much on the code side, but I think the default palette in this case should be changed.

@nickofthyme nickofthyme requested a review from a team as a code owner August 23, 2024 15:39
Comment on lines +563 to +570
getTelemetryEventsOnSave(state, prevState) {
const colorMappingEvents = state.columns.flatMap((col) => {
const prevColumn = prevState?.columns?.find((prevCol) => prevCol.columnId === col.columnId);
return getColorMappingTelemetryEvents(col.colorMapping, prevColumn?.colorMapping);
});

return colorMappingEvents;
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@teresaalvarezsoler did you want to update these telemetry events to include the vis type they were used in? Currently all events are the same across xy, partition, tagcloud and now datatable. Example of the current events are...

  • lens_color_mapping_palette_eui_amsterdam_color_blind
  • lens_color_mapping_unassigned_terms_neutral
  • lens_color_mapping_colors_up_to_2
  • lens_color_mapping_avg_count_terms_per_color_1

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

limits.yml

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 an no bugs found! 🎉

Left only a minor code nitpick which would be nice to address. For the rest great work @nickofthyme ! 🚀

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Approving to unblock - still interested to see what @elastic/kibana-design says about the positioning of this tooltip icon, but definitely not worth blocking all this work 🙇

Current Option 1 - keep style, vertically align Option 2 - match style of controls tooltip icon
image image image

cc @MichaelMarcialis

@@ -15,22 +15,26 @@ import { Container } from './components/container/container';
import { ColorMapping } from './config';
Copy link
Contributor

Choose a reason for hiding this comment

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

I observed this small bug when switching between categorical/numerical color palettes.

  1. Create a datatable with a last value dimension and categorical color palette set.
  2. switch to a numerical operation, like 'sum'
  3. Click on the color palette - the palette input is not filled properly. I'll pass you the video in a PM (github limits won't allow me to do so)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed this in 77cd409

nickofthyme and others added 2 commits August 26, 2024 13:24
Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes, @nickofthyme. Everything looks great. I left two final nits below, but nothing worth holding you up over. Assuming you're able to address them, I'll approve now.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1505 1513 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/chart-expressions-common 19 21 +2
@kbn/coloring 180 188 +8
lens 583 586 +3
total +13

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 546.7KB 547.3KB +643.0B
expressionXY 129.3KB 129.3KB +62.0B
lens 1.5MB 1.5MB +7.1KB
total +7.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
expressionPartitionVis 29.0KB 29.1KB +168.0B
expressionTagcloud 13.1KB 13.2KB +168.0B
expressionXY 41.7KB 41.9KB +168.0B
fieldFormats 63.0KB 63.1KB +114.0B
lens 50.9KB 51.6KB +764.0B
total +1.3KB
Unknown metric groups

API count

id before after diff
@kbn/chart-expressions-common 23 25 +2
@kbn/coloring 217 227 +10
lens 685 688 +3
total +15

ESLint disabled line counts

id before after diff
lens 16 17 +1

Total ESLint disabled count

id before after diff
lens 16 17 +1

History

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

@nickofthyme nickofthyme merged commit a994629 into elastic:main Aug 28, 2024
20 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Aug 28, 2024
wajihaparvez added a commit that referenced this pull request Nov 11, 2024
## Summary

Added a section with instructions for assigning colors to terms in a
table. The technical preview warning will be removed in GA.

Rel: #189895
Closes: [#559](elastic/platform-docs-team#559)

---------

Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 11, 2024
## Summary

Added a section with instructions for assigning colors to terms in a
table. The technical preview warning will be removed in GA.

Rel: elastic#189895
Closes: [elastic#559](elastic/platform-docs-team#559)

---------

Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
(cherry picked from commit b781c4e)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 11, 2024
## Summary

Added a section with instructions for assigning colors to terms in a
table. The technical preview warning will be removed in GA.

Rel: elastic#189895
Closes: [elastic#559](elastic/platform-docs-team#559)

---------

Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
(cherry picked from commit b781c4e)
tkajtoch pushed a commit to tkajtoch/kibana that referenced this pull request Nov 12, 2024
## Summary

Added a section with instructions for assigning colors to terms in a
table. The technical preview warning will be removed in GA.

Rel: elastic#189895
Closes: [elastic#559](elastic/platform-docs-team#559)

---------

Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Nov 18, 2024
## Summary

Added a section with instructions for assigning colors to terms in a
table. The technical preview warning will be removed in GA.

Rel: elastic#189895
Closes: [elastic#559](elastic/platform-docs-team#559)

---------

Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
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:feature Makes this part of the condensed release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Color options for string cells in tables