refactor(partition): remove config in favor on spec and theme controls #1402

nickofthyme · 2021-09-22T16:23:59Z

Summary

Adds true dark mode to partition charts with configurable theme styles.

BREAKING CHANGES

The PartitionSpec.config prop is removed. All properties have been moved/renamed under new Theme.partition options or PartitionSpec itself with the following exceptions:

PartitionSpec.config.margin is removed in favor of Theme.chartPaddings. This no longer supports ratios.
PartitionSpec.config.backgroundColor is removed in favor of Theme.background.color.
PartitionSpec.config.width and PartitionSpec.config.height both removed as they were never used.

Details

Up to now Partition charts have had no connection to the chart Theme and thus had no canonical inheritance to dark or light theming modes. This PR fixes this connection. PartitionSpec.config was broken up and moved into Theme, the PartitionSpec or deleted, see details in table below.

Config prop	Moved to `Theme.partition`	Moved to `PartitionSpec`	Unused (removed)	Remarks
`width`			✅	Always overridden by parent dimensions
`height`			✅	Always overridden by parent dimensions
`margin`	✅			No longer a ratio, use `Theme.chartMargins` or `Theme.chartPaddings`
`emptySizeRatio`	✅
`outerSizeRatio`	✅
`clockwiseSectors`		✅
`specialFirstInnermostSector`		✅
`partitionLayout`		✅		renamed to `layout`
`drilldown`		✅
`fontFamily`	✅
`circlePadding`	✅
`radialPadding`	✅
`horizontalTextAngleThreshold`	✅
`horizontalTextEnforcer`	✅
`maxRowCount`		✅
`fillOutside`		✅
`radiusOutside`		✅
`fillRectangleWidth`		✅
`fillRectangleHeight`		✅
`fillLabel`	✅
`fillLabel.valueFormatter`			✅	formatter was always overriden by config default. Use `valueFormatter` on `layers` or `spec`.
`linkLabel`	✅
`linkLabel.valueFormatter`			✅	formatter was always overriden by config default. Use `valueFormatter` on `layers` or `spec`.
`backgroundColor`			✅
`sectorLineWidth`	✅
`sectorLineStroke`	✅

Changes also add per theme gap stroke color and fix #1499, see demo below...

Issues

fix #1185, fix #1499

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
All related issues have been linked (i.e. closes #123, fixes #123)
New public API exports have been added to packages/charts/src/index.ts
Unit tests have been added or updated to match the most common scenarios
The proper documentation and/or storybook story has been added or updated
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

monfera · 2021-09-23T14:40:48Z

The reason for the config object was/is, automatic generation of config UI: runtime detectable value types, sensible number ranges for sliders and enum strings a UI would conceivably put into a dropdown, and of course booleans for checkboxes. Here's an example:

Over time, some of these powers could be provided to Elastic users for direct tweaks, and would be useful for us too, and for our designers so they can quickly iterate on color, font size, line width, eventual animation parameters and other things.

It's not fleshed out in the code, the thinking became, class objects would be a good representation for the props. For class instances are the only composite data types in JS that are expressively typed in runtime (eg. instanceof) and statically (the classes double as TypeScript interfaces) even if I'm not a big fan of classes

nickofthyme · 2021-09-23T19:56:18Z

The reason for the config object was/is, automatic generation of config UI

@monfera yeah I realize that but this has bleed into the lack of coupling between charts and theme, requiring chart users to define options that should be on theme in config every time they create a chart, which is not scalable.

// Example I've seen in kibana for heatmap and partition charts
function MyPieChart() {
  const theme = useTheme();
  const config = {
    linkLabel: {
      textColor: theme.textColor // not real
    },
  };

  return (
    <Chart>
      <Settings theme={theme} />
      <Partition config={config} />
    </Chart>
  );
}

I am 100% onboard creating a robust config that not only facilitates an options UI like you show above but also apply validation to provided values at lookup time or when instantiated. This is why I kept all the config meta data. Having a config like this in the future, maybe in playground to start, would be easy to configure such that it feeds config options to spec or theme accordingly based on chart type.

Without validation these meta data objects are inflating the bundle size while serving no purpose outside of development. This is something we could also keep in mind for the future.

Happy to discuss further with you to still leverage the UI with sliders.

…controls

…l formatters

nickofthyme

Just a few comments to explain some changes.

.eslintrc.js

package.json

packages/charts/src/chart_types/partition_chart/specs/index.ts

packages/charts/src/common/color_calcs.test.ts

nickofthyme · 2021-09-24T15:39:44Z

packages/charts/src/common/fill_text_color.ts

+export function fillTextColor(
+  foreground: Color | null,
+  background: Color = Colors.Transparent.keyword,
+  fallbackColor?: Color,
+): Color {
  const backgroundRGBA = colorToRgba(background);
-  const containerBgRGBA = combineColors(colorToRgba(containerBg), Colors.White.rgba); // combine it with white if semi-transparent
-  const blendedFbBg = combineColors(backgroundRGBA, containerBgRGBA);
-  return RGBATupleToString(highContrastColor(blendedFbBg));
+  if (backgroundRGBA[3] < TRANSPARENT_LIMIT && !foreground && fallbackColor) return fallbackColor;
+
+  if (foreground) {
+    const foregroundRGBA = colorToRgba(foreground);
+
+    if (backgroundRGBA[3] < TRANSPARENT_LIMIT) {
+      if (foregroundRGBA[3] < TRANSPARENT_LIMIT && fallbackColor) return fallbackColor;
+      // combine it with white if semi-transparent
+      const fgBlend = foregroundRGBA[3] < 1 ? combineColors(foregroundRGBA, Colors.White.rgba) : foregroundRGBA;
+      // only use foreground
+      return RGBATupleToString(highContrastColor(fgBlend));
+    }
+
+    // combine it with white if semi-transparent
+    const bgBlend = combineColors(backgroundRGBA, Colors.White.rgba);
+    const blendedFgBg = combineColors(foregroundRGBA, bgBlend);
+    return RGBATupleToString(highContrastColor(blendedFgBg));
+  }
+
+  // combine it with white if semi-transparent
+  const bgBlend = combineColors(backgroundRGBA, Colors.White.rgba);
+  return RGBATupleToString(highContrastColor(bgBlend));
 }


@markov00 I am proposing changing this logic to account for a fallback value. If the foreground and background colors are not suitable and there is a fallback color (e.g. color from the theme), then we use that instead of attempting to blend with white on unknown theme. This works the same as before when using none transparent colors.

I also renamed the parameters to be more inline with the combineColors naming where the foreground such as pie slice or bar color, can be null and the background color (was container color) is consistently the background color, before these were interchangeable.

Finally the background color is defaulted to transparent to allow the fallback usage where before this was using a value that was typically defaulted to white from outside this function.

This is mainly around the case where the background is not defined or transparent and we are guessing the color, typically favoring light themes. Before we must have defined the correct background color for dark themes for this to work and now it can use the fallback textColor from the theme definition.

I have few comments on that:

instead of this code change, we can always use of the theme textColor for both the linked labels and the links lines. This "ensures" that the theme builder must consider the linkedLabel color independently from the theme background. If used with a transparent background, the linkedLabel should be manually adapted by the user depending on the real background used (within Canvas for example).

taking into account what is described in 1) the fillTextColor will then only be used to compute colors within slices/sectors.
From the code, it looks like that the theme textColor is used only when a highly transparent (<0.1 alpha) slice/sector and a highly transparent background are combined. I'm not 100% sure but this doesn't fully solve the problem because a singletextColor is not always highly contrasting with their background.
The problem here is more on the "missing" background of the pie/slice/sector. There is a use case (Kibana Canvas) where we don't want to draw a piechart with linked labels with an opaque background because, probably, we want to draw some text/icons around it. In this case, where the theme.background.color is transparent, we have a problem. What if, instead of providing a fallback color for the text, we provide an opaque background color for the pie/sector? this color will be used the same way as the container background, but it will not be rendered on the canvas itself, because we rely on the theme.background.color or on the actual background color of the chart element.

Combining both 1) and 2) means replace the textColor from the fillLabel style with something like opacqueBackgroundColor or something along these lines, and we then use the function fillTextColor(foreground,opacqueBackgroundColor) only for the filled labels, not for the linked ones

@markov00 other than this vrt diff solving #1499 (see below), it looks like the vrt show no changes to the color fill logic.

What about merging this and revisiting the color logic in a future pr? Or I can revert them and just add the changes in another PR.

packages/charts/src/components/accessibility/accessibility.test.tsx

packages/charts/src/utils/themes/dark_theme.ts

monfera · 2021-09-27T19:56:09Z

Thanks Nick, your explanation makes sense, and the UI generation aspect is not something we exercise currently. It's not difficult to bring it back at some point, so shedding it for now is a good call

nickofthyme · 2021-09-27T22:59:57Z

Thanks Nick, your explanation makes sense, and the UI generation aspect is not something we exercise currently. It's not difficult to bring it back at some point, so shedding it for now is a good call

Yeah I am jealous every time I see you using the UI generation cuz is seems so simple and practical!

nickofthyme · 2021-09-27T23:40:00Z

Most if not all partition chart VRT screenshots changed after removing config.margin and adding chartPaddings and chartMargins. I could set all these to 0 for sake of matching the screenshots and update just the padding in a follow up PR if people think that's better.

Also still looking into visual regressions due to:

missing donut center
larger text on flame charts
reversed direction of slice order CW/CCW.

monfera · 2021-09-28T11:59:20Z

update just the padding in a follow up PR

I think it'd help a bit, so it's harder to miss some unintended change, though if it's hard to separate, it's fine either way

- revert unintended changes from config refactor - set all chart margins for partition charts to 0 as before - update all screenshots

nickofthyme

@monfera do you know why the flame and icicle text is bigger? I cannot see any changes that would cause this.

monfera · 2021-10-05T21:34:56Z

@nickofthyme just eyeballing the flame image diffs: it's not just that the font became bigger; also it looks like it's not using variable sized fonts. The idea is that within the minimum and maximum font size, we allow a range of font sizes, visible on the left but not present on the right

While sometimes the font sizes—even variable ones—look identical, sometimes there's significant difference, eg. often, smaller fonts in the PR:

There seem to be a stroke related regression here: the old one has a stroke color that appears identical with the background (which is a design goal) while in the feature branch, the stroke is visibly darker, easiest to see if you move the slider

Some charts shift a bit

While the upward movement of the legends looks like a good change, it masks other changes (as in, the reviewer may not do the slider thing, or not attentively, on all the dozens of images and some unexpected changes remain veiled)

The waffle shrank a bit for some reason:

All these look OK visually (except the font size and the stroke issues), it's just I'm uncertain why some things changed the way they did.

The newly added dark background examples look fantastic 🎉

nickofthyme · 2021-10-25T18:46:10Z

@monfera

it's not just that the font became bigger; also it looks like it's not using variable sized fonts. The idea is that within the minimum and maximum font size, we allow a range of font sizes, visible on the left but not present on the right.

Ok thanks I'll take a look at this logic.

While sometimes the font sizes—even variable ones—look identical, sometimes there's significant difference, eg. often, smaller fonts in the PR:

Yeah I'll take a look, though seems there needs to be logic to check the scaled down text for small multiples before comparing against min font size. Future thing...

There seem to be a stroke related regression here

The idea of this PR is to add theming that is not governed by the contrast logic. So in this case the line stroke is now defined explicitly on the theme for dark and light mode. This is likely the difference you see in this case.

Some charts shift a bit

Yes with the changes to the margin, namely removing the margin ratio, there is some shift in the margin where the hardcoded margin is close but not exactly the same as before.

As for the 🧇 chart, This is slightly difference because the waffle uses chart padding to scale down the chart inside the chart area, before this was hardcoded with ratio values. Now by default the waffle chart will fill the available space like all other partition charts, and xy charts for that matter.

monfera · 2021-11-09T19:13:14Z

@nickofthyme Thanks for your explanation, I now understand that some things changed in line with your expectation, even if the change doesn't look desirable, like making the waffle smaller.

In case of the stroke issue, the chart looks bad if the stroke and background colors do not match, even if there's a reasonable technical explanation for the change. As the stories are also copy/pasteable examples for making specific charts, it'd be better if it was restored to something sensible. Ideally we also avoid story charts that have obvious, or easy to avoid subtle design problems with them

nickofthyme · 2021-11-09T21:13:40Z

@monfera I don't see a grossly different stroke. Can you point out what exactly your are seeing? Thanks.

…hots

nickofthyme · 2021-12-16T22:36:28Z

@monfera It appears the flame font increase was caused because the flame chart never pulled the max value from the config. Now with it sharing the config the max font value was set to 64 across all partition types and caused the increase. Leaving this for now and will just have to set this as a manual override on the chart if desired. See 37f4349.

packages/charts/src/chart_types/partition_chart/layout/config.ts

- add top-level background color option in theme - add theme validation for fallback bg color - replace fallback text color with bg color - use bg color in bar value text logic - update outdatad unit tests

nickofthyme added enhancement New feature or request :styling Styling related issue :partition Partition/PieChart/Donut/Sunburst/Treemap chart related breaking change :theme labels Sep 22, 2021

nickofthyme mentioned this pull request Sep 23, 2021

chore: merge alpha into master #1403

Closed

nickofthyme added 4 commits September 23, 2021 18:56

refactor(partition_charts): remove config in favor on spec and theme …

c378f7c

…controls

docs: update stories with api changes

700c573

chore: merge config and theme types, remove unused link and fill labe…

d738187

…l formatters

fix: add fallback color to color contrast logic

792323a

nickofthyme force-pushed the partition-theme branch from 3c5f5af to 792323a Compare September 24, 2021 02:36

nickofthyme added 5 commits September 24, 2021 08:36

fix: contrast logic default bg color

aef3d0a

chore: add chart padding to partitions, update all screenshots

8e312ce

chore: fix prettier errors

8471ebf

test: add test for contrast logic changes

85b60b3

fix: api errors and ts version issues

215a21f

nickofthyme marked this pull request as ready for review September 24, 2021 15:25

chore: fix api syntax error

eb37a98

nickofthyme requested review from monfera and markov00 September 24, 2021 15:28

nickofthyme commented Sep 24, 2021

View reviewed changes

nickofthyme added 2 commits September 29, 2021 09:33

Merge branch 'master' into partition-theme

08beaeb

test: revert chages to vrt

9cdb22e

- revert unintended changes from config refactor - set all chart margins for partition charts to 0 as before - update all screenshots

nickofthyme commented Sep 29, 2021

View reviewed changes

nickofthyme mentioned this pull request Nov 8, 2021

[Partition] Link label text color parameter passed is ignored #1469

Closed

2 tasks

Merge branch 'master' into partition-theme

9660fc0

monfera changed the title ~~refactor(partition_charts): remove config in favor on spec and theme controls~~ refactor(partition): remove config in favor on spec and theme controls Nov 9, 2021

nickofthyme added 9 commits December 16, 2021 07:59

Merge branch 'master' into partition-theme

9bd0bdc

chore: fix api chages

4d8328e

chore: rename and export default partition formatter

e29f769

chore: export PartitionStyle type

35f1d85

chore: update color blend logic

599e540

fix: linting errors

4872465

chore: add new textColor fallback to partition theme, fix vrt screens…

0eeaac8

…hots

test: remove padding on bar chart

edd2112

test: reduce flame chart maxFontSize on vrt

37f4349

nickofthyme linked an issue Dec 16, 2021 that may be closed by this pull request

Color contrast on dark theme show invisible link labels #1499

Closed

chore: update api with textColor fallback prop

1272c7e

nickofthyme requested a review from markov00 December 17, 2021 14:26

test: update flame min font size to match previous

ba31d3e

markov00 approved these changes Dec 17, 2021

View reviewed changes

packages/charts/src/chart_types/partition_chart/layout/config.ts Show resolved Hide resolved

nickofthyme added 2 commits December 17, 2021 14:52

refactor: color logic to use fallback bg color

1032416

- add top-level background color option in theme - add theme validation for fallback bg color - replace fallback text color with bg color - use bg color in bar value text logic - update outdatad unit tests

fix: heatmap contrast logic and verbiage

3790580

nickofthyme merged commit 6db2677 into elastic:master Dec 17, 2021

nickofthyme deleted the partition-theme branch December 17, 2021 22:09

nickofthyme mentioned this pull request Dec 19, 2021

Partitioning visualizations - improvement ideas #518

Open

11 tasks

monfera mentioned this pull request Dec 29, 2021

[Flamegraph] tweening stopped (regression) #1540

Closed

3 tasks

nickofthyme mentioned this pull request Apr 12, 2023

[Lens] Pie/Treemap chart external labels are hard to read when in dark mode elastic/kibana#116777

Closed

nickofthyme mentioned this pull request Jun 1, 2023

docs: remove erroneous zero margins/paddings #2055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(partition): remove config in favor on spec and theme controls #1402

refactor(partition): remove config in favor on spec and theme controls #1402

nickofthyme commented Sep 22, 2021 •

edited

Loading

monfera commented Sep 23, 2021

nickofthyme commented Sep 23, 2021 •

edited

Loading

nickofthyme left a comment

nickofthyme Sep 24, 2021

nickofthyme Sep 24, 2021 •

edited

Loading

markov00 Oct 5, 2021

nickofthyme Dec 16, 2021 •

edited

Loading

monfera commented Sep 27, 2021

nickofthyme commented Sep 27, 2021

nickofthyme commented Sep 27, 2021 •

edited

Loading

monfera commented Sep 28, 2021

nickofthyme left a comment

monfera commented Oct 5, 2021 •

edited

Loading

nickofthyme commented Oct 25, 2021

monfera commented Nov 9, 2021

nickofthyme commented Nov 9, 2021

nickofthyme commented Dec 16, 2021

refactor(partition): remove config in favor on spec and theme controls #1402

refactor(partition): remove config in favor on spec and theme controls #1402

Conversation

nickofthyme commented Sep 22, 2021 • edited Loading

Summary

BREAKING CHANGES

Details

Issues

Checklist

monfera commented Sep 23, 2021

nickofthyme commented Sep 23, 2021 • edited Loading

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Sep 24, 2021

Choose a reason for hiding this comment

nickofthyme Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

markov00 Oct 5, 2021

Choose a reason for hiding this comment

nickofthyme Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

monfera commented Sep 27, 2021

nickofthyme commented Sep 27, 2021

nickofthyme commented Sep 27, 2021 • edited Loading

monfera commented Sep 28, 2021

nickofthyme left a comment

Choose a reason for hiding this comment

monfera commented Oct 5, 2021 • edited Loading

nickofthyme commented Oct 25, 2021

monfera commented Nov 9, 2021

nickofthyme commented Nov 9, 2021

nickofthyme commented Dec 16, 2021

nickofthyme commented Sep 22, 2021 •

edited

Loading

nickofthyme commented Sep 23, 2021 •

edited

Loading

nickofthyme Sep 24, 2021 •

edited

Loading

nickofthyme Dec 16, 2021 •

edited

Loading

nickofthyme commented Sep 27, 2021 •

edited

Loading

monfera commented Oct 5, 2021 •

edited

Loading