[Lens] Make metric color fill the entire space

[dej611]

Summary

Fixes #120387

Make the metric fill behaviour use the entire space of the visualization panel.

Note: the metric title will flip from pure black to pure white depending on the theme.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

[dej611]

@elasticmachine merge upstream

[dej611]

@elasticmachine merge upstream

[dej611]

@elasticmachine merge upstream

[stratoula]

Although I really like the option to fill the entire panel, this will mean that all the existing metrics with fill, will change.
Instead of filling only the value, now the users with existing metric visualizations will see a different coloring format. Maybe they don't want it. What I want to say, is that by changing the current behavior maybe creates some confusion to the user that they already have metric viz to their dashboards.

Wdyt @flash1293 @ghudgins ?

[dej611]

But dynamic color for metric has been added in this same release: #116170, so no user is currently using it.

[stratoula]

You are correct!! I forgot it! Then yes I see no prob with that! I will review it asap :)

[stratoula]

I definitely prefer it from its viz editor equivalent :D

Code LGTM, I tested it locally in Chrome and it seems fine!

[dej611]

@ghudgins @stratoula you think it makes sense to offer also the "reduced" version as for the other vis editor or is it ok to keep this version only?

[stratoula]

The previous implementation worked like that

It doesn't look exactly the same with the viz editor as it colours only the value and not the label. Honestly I don't have a strong opinion, whatever @ghudgins prefers. My proposal is to merge this PR and if the users ask for this option, then add it. It is a new feature after all :)

[ghudgins]

@stratoula agree with that logic. I don't feel strongly about including it in initial release now that we have the more desirable panel option. I pinged @gvnmagni as well just to make sure

[gvnmagni]

@stratoula agree with that logic. I don't feel strongly about including it in initial release now that we have the more desirable panel option. I pinged @gvnmagni as well just to make sure

Agree! Since it doesn't add any value and it offers a worse version of the solution we are proposing I'd say that if we don't have anything holding us back we can go with the first option only.

[MichaelMarcialis]

@dej611: This looks to be a nice change over the previous behavior. Well done. The PR looks good in general, but I had a handful of questions before approving. CCing @ghudgins and @gvnmagni, in case they have an opinion.

1. Would it be possible to remove or compensate for the container padding in Lens (top/right/bottom/left) and Dashboard (right/bottom/left) so that the background color is flush with the edge of the visualization's container? I think doing so would help avoid the box-in-a-box look and reduce visual tension.

2. Not super important, but I had a minor question/thought about the consistency of text colors within the metric visualization type with these changes. When no fill color is applied to a metric visualization (in light mode), the metric value color is the standard EUI text color ($euiTextColor), but the display name is black (#000). However, when a fill is applied, both the metric value and display name receive the same color (either black or white depending on the chosen fill color). For the sake of consistency, should the metric value and display name be using the same color when no fill is applied?

3. Similar to the above, unlike these recent changes to the fill behavior, the option to color text by value only applies those colors to the metric value text. For the sake of consistency, should the color be applied to both the metric value and display name? For the sake of accessibility, my first instinct is to say keep it as we currently have it (as not applying the user-selected color to the display name text allows us to ensure proper contrast), but thought I'd ask the group.

[dej611]

2. Not super important, but I had a minor question/thought about the consistency of text colors within the metric visualization type with these changes. When no fill color is applied to a metric visualization (in light mode), the metric value color is the standard EUI text color ($euiTextColor), but the display name is black (#000). However, when a fill is applied, both the metric value and display name receive the same color (either black or white depending on the chosen fill color). For the sake of consistency, should the metric value and display name be using the same color when no fill is applied?

That is definitely possible. My idea was to not change the look and feel for the existing metrics.
You think would be ok to make everything pure black ( #000)?

Similar to the above, unlike these recent changes to the fill behavior, the option to color text by value only applies those colors to the metric value text. For the sake of consistency, should the color be applied to both the metric value and display name? For the sake of accessibility, my first instinct is to say keep it as we currently have it (as not applying the user-selected color to the display name text allows us to ensure proper contrast), but thought I'd ask the group.

My idea was to prevent readability issues if the user would set very light colors on the metric, hence keep the display name out of the coloring in this case.

[dej611]

Would it be possible to remove or compensate for the container padding in Lens (top/right/bottom/left) and Dashboard (right/bottom/left) so that the background color is flush with the edge of the visualization's container? I think doing so would help avoid the box-in-a-box look and reduce visual tension.

I've tried to look into that, but it seems that the padding is added in the Expression renderer where there's no knowledge of the metric configuration.

[MichaelMarcialis]

Would it be possible to remove or compensate for the container padding in Lens (top/right/bottom/left) and Dashboard (right/bottom/left) so that the background color is flush with the edge of the visualization's container? I think doing so would help avoid the box-in-a-box look and reduce visual tension.

I've tried to look into that, but it seems that the padding is added in the Expression renderer where there's no knowledge of the metric configuration.

Gotcha. Thanks for looking into it.

Not super important, but I had a minor question/thought about the consistency of text colors within the metric visualization type with these changes. When no fill color is applied to a metric visualization (in light mode), the metric value color is the standard EUI text color ($euiTextColor), but the display name is black (#000). However, when a fill is applied, both the metric value and display name receive the same color (either black or white depending on the chosen fill color). For the sake of consistency, should the metric value and display name be using the same color when no fill is applied?

That is definitely possible. My idea was to not change the look and feel for the existing metrics. You think would be ok to make everything pure black ( #000)?

Assuming others feel the same or are indifferent to such a change, I'd say make them both $euiTextColor. But again, it's a small thing, so if others would prefer we keep as is, I'm fine with that.

Similar to the above, unlike these recent changes to the fill behavior, the option to color text by value only applies those colors to the metric value text. For the sake of consistency, should the color be applied to both the metric value and display name? For the sake of accessibility, my first instinct is to say keep it as we currently have it (as not applying the user-selected color to the display name text allows us to ensure proper contrast), but thought I'd ask the group.

My idea was to prevent readability issues if the user would set very light colors on the metric, hence keep the display name out of the coloring in this case.

Yeah, sounds like your feel the same way I do (re: accessibility). In that case, let's keep it as you have it (with color-by-value on text only being applied to the metric value).

That said, I'll approve now so I don't hold you up further.

[dej611]

@elasticmachine merge upstream

[dej611]

@elasticmachine merge upstream

[dej611]

@MichaelMarcialis updated the PR using the euiTextColor as dark color in light theme and as light color in dark theme.

I guess this could be extended in a follow up also to other visualizations (i.e. table & heatmap).

[dej611]

@elasticmachine merge upstream

[flash1293]

@elasticmachine merge upstream

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 7593159
• Storybooks Preview

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.0MB	1.0MB	+114.0B

History

• 💚 Build #17728 succeeded 6823655
• 💚 Build #16956 succeeded a2f8cd1
• 💚 Build #16336 succeeded 6124392
• 💚 Build #15615 succeeded 5a081c0
• 💚 Build #15532 succeeded c4c4fdb
• 💚 Build #15455 succeeded a9cf810

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

[flash1293]

Tested and works as expected, LGTM