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

feat(metric): allow alpha colors and improve contrast logic #2184

Merged
merged 23 commits into from
Oct 10, 2023

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Oct 2, 2023

Summary

Adds support for colors with alpha channel on Metric charts including sparkline and progress bar colors.

Screen.Recording.2023-10-02.at.01.50.54.PM.mp4

It's hard to see the changes from a simple demo so please play around with the story here.

Details

These changes clean up a bit of the background color and contrast color for the Metric chart.

Notable changes:

  • Removes the backgroundColor option on the Theme.metric (aka MetricStyle) in favor of the top-level Theme.background
  • Fixes color logic converting hsl to rgba while persisting the alpha value, see 48f2076.
  • Enables setting transparent colors for progress bar, overall metric color and implicitly the sparkline color.

Issues

Fixes #2183

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

- allow passing alpha channel for metric color & sparkline
- allow passing alpha channel for metric progress bar color
- remove backgroundColor from metric in favor of top-level bgColor
-
@nickofthyme nickofthyme added :styling Styling related issue :metric Related to Metric chart labels Oct 2, 2023
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme nickofthyme marked this pull request as ready for review October 3, 2023 21:37
@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@@ -36,8 +36,8 @@ export const SparkLine: FunctionComponent<{
trendShape === MetricTrendShape.Bars ? CurveType.CURVE_STEP_AFTER : CurveType.LINEAR,
);

const [h, s, l] = colorToHsl(color);
const pathColor = hslToColor(h, s, l >= 0.8 ? l - 0.1 : l + 0.1);
const [h, s, l, a] = colorToHsl(color);
Copy link
Member

Choose a reason for hiding this comment

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

I think you have to blend first the color with a valid opaque background before changing the lightning, preserving the alpha is not the same thing as we where looking at doing here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not quite sure what you're saying here, I need to blend the sparkline color with an opaque background?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you are right here since the transparent sparkline would be rendered on top of the transparent back area. I think a good fix of this is to just mask the filled sparkline from the background. See 6bd75a4

Screen Recording 2023-10-06 at 02 14 49 PM

Comment on lines 97 to 105
const highContrastTextColor = fillTextColor(
backgroundColor,
isMetricWProgress(datum) ? backgroundColor : datum.color,
undefined,
{
lightColor: colorToRgba(style.text.lightColor),
darkColor: colorToRgba(style.text.darkColor),
},
);
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should improve this a bit because the text could be above the background but also above the area background that has a different luminosity and that could change the contrast.
I think we can do the following:

  • find the color contrast ratios of each text color for both background colors (metric and area)
  • pick the text color that is more close to pass the WCAG threshold in both situations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you talking about the checking between the background and the sparkline area?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I took a stab at this and indeed there is a small range where the text color contrast selection is opposing.

So I'm checking when there is a trend line for the contrast over the sparkline, then if different I take the color with the higher ratio. This is the part I'm not sure about, if it should be the higher one but also if we are even comaring the right ration since one is based on the light color and the other on the dark.

The main logic is here...

const highContrastTextColor = fillTextColor(
backgroundColor,
isMetricWProgress(datum) ? backgroundColor : datum.color,
undefined,
contrastOptions,
);
let finalTextColor = highContrastTextColor.color;
if (isMetricWTrend(datum)) {
const { ratio, color, shade } = fillTextColor(
backgroundColor,
getSparkLineColor(datum.color),
undefined,
contrastOptions,
);
// TODO verify this check is applied correctly
if (shade !== highContrastTextColor.shade && ratio > highContrastTextColor.ratio) {
finalTextColor = color;
}
}

@nickofthyme
Copy link
Collaborator Author

buildkite update screenshots

@nickofthyme nickofthyme merged commit dd5732e into elastic:main Oct 10, 2023
13 checks passed
@nickofthyme nickofthyme deleted the fix-metric-opacity branch October 10, 2023 23:30
@nickofthyme nickofthyme changed the title feat: allow alpha colors and improve contrast logic feat(metric): allow alpha colors and improve contrast logic Oct 10, 2023
nickofthyme pushed a commit that referenced this pull request Nov 8, 2023
# [61.0.0](v60.0.0...v61.0.0) (2023-11-08)

### Bug Fixes

* `onRenderChange` callback trigger on resize ([#2228](#2228)) ([be30c1b](be30c1b))
* **axis:** always render `tickLine` unless `visible` is `false` ([#2194](#2194)) ([ec95d50](ec95d50))
* **BarSeries:** ignore histogram mode in determining stacked series ([#2225](#2225)) ([27b4281](27b4281))
* clamp brushing min of last bucket ([#2227](#2227)) ([155c22d](155c22d))
* **deps:** update dependency @elastic/eui to ^88.5.0 ([#2179](#2179)) ([2bb921e](2bb921e))
* **deps:** update dependency @elastic/eui to ^88.5.4 ([#2190](#2190)) ([05b33e5](05b33e5))
* **deps:** update dependency @elastic/eui to ^89.1.0 ([#2212](#2212)) ([a91f68d](a91f68d))
* **deps:** update dependency @elastic/eui to v89 ([#2193](#2193)) ([132327d](132327d))
* **deps:** update dependency @elastic/eui to v90 ([#2222](#2222)) ([10cd53b](10cd53b))

### chore

* reclaim charts theme ownership from eui ([#2175](#2175)) ([422c7d5](422c7d5))

### Features

* **metric:** allow alpha colors and improve contrast logic  ([#2184](#2184)) ([dd5732e](dd5732e))

### BREAKING CHANGES

* **BarSeries:** now ignores histogram mode in determining stacked series
* elastic charts theme renamed to `LEGACY_DARK_THEME` and `LEGACY_LIGHT_THEME` in favor of the main `DARK_THEME` and `LIGHT_THEME` which was merged with eui theme overrides. These new themes are now default.
* **axis:** Now respects `tickLine.padding` whenever `tickLine.visible` is `true`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:metric Related to Metric chart :styling Styling related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metric] Opacity is ignored when passed as RGBA or HEX string
3 participants