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] Fix Treemap outer labels with transparent background #84245

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 24, 2020

Summary

Fixes #83525

The outer layer has a transparent background, precisely a rgba(0,0,0,0) (note 0 opacity), which makes the elastic charts library produce a white text color. The library ignores the alpha parameter when computing the contrast color, so it requires a hint about dark mode.
This PR makes the component aware of the dark mode theme state and hints the chart library about the Kibana background color.
Tested on Lens + Dashboard

Screenshot 2020-11-24 at 16 40 25

Screenshot 2020-11-24 at 16 40 33

Screenshot 2020-11-24 at 16 41 23

@dej611 dej611 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.11.0 labels Nov 24, 2020
@dej611 dej611 requested a review from a team November 24, 2020 16:05
@elasticmachine
Copy link
Contributor

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

@dej611
Copy link
Contributor Author

dej611 commented Nov 24, 2020

Notice that the fix will not automatically work in Canvas:

Screenshot 2020-11-24 at 17 41 12

Screenshot 2020-11-24 at 17 35 29

The background color hint passed to the library does not cover Canvas page "background".

@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 937.0KB 937.1KB +172.0B

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

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

This PR fixes the severe bug, so I think it's a step in the right direction. Tested in Chrome, code LGTM

However, I don't like that we introduce another 'problem' in Canvas when Kibana is in the dark mode, but canvas has white background. In the future, with more customisability options maybe we'll allow users to fix it by using their own background color or font color, or we'll pass a background from canvas to set the proper color for the text.

@dej611
Copy link
Contributor Author

dej611 commented Nov 25, 2020

Passing the Canvas Page background color to the embeddable would be great, in general I think, agree.
Also more customisation would help as well on this side.

@dej611 dej611 merged commit 6cd4d84 into elastic:master Nov 25, 2020
@dej611 dej611 deleted the fix/83525 branch November 25, 2020 09:29
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 27, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 84245 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Outer labels invisible on nested treemap
4 participants