feat(style): allow fill and stroke overrides #258

markov00 · 2019-07-03T13:32:10Z

Summary

This PR allows the computed series color to be overridden by the theme or the series specific style.

BREAKING CHANGE: LineStyle, AreaStyle and BarSeriesStyle types differs on the optional values.
stroke and fill on the theme or specific series style now override the computed series color.

The a style overrides with the following flows:

if a style is declared at the Spec level (on a Bar/Area/Line component) it will take precedence over all the other style (any stroke or fill prop will override the computed series color for that specific style element). If not go to next
if a stroke or fill is present on the chart theme prop for a Bar/Area/Line than it will use that color for the point (stroke or fill) line (stroke) bar(stroke, fill, borderStroke)
if no stroke or fill is configured on theme or series specific style than we will use the one computed or assigned with customSeriesColors prop

notes

I've refactored the build...props used to compute the props for rendering lines/areas/bars/points, cleaning a but the function semantics.
The payload of each individual bar rendered geometry is currently increased, but I will find a better way to specify that for bars in the future.
I've cleaned the defaults of the two themes
I've cleaned the stories for custom styles to align it with that new feature

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility, including a check against IE11
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios
Each commit follows the convention

This allows the computed series color to be overridden by the theme or the series specific style. BREAKING CHANGE: `LineStyle`, `AreaStyle` and `BarSeriesStyle` types differs on the optional values. `stroke` and `fill` on the theme or specific series style now override the computed series color.

codecov-io · 2019-07-03T13:52:16Z

Codecov Report

Merging #258 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
+ Coverage   97.84%   97.99%   +0.14%     
==========================================
  Files          36       36              
  Lines        2693     2840     +147     
  Branches      607      660      +53     
==========================================
+ Hits         2635     2783     +148     
  Misses         51       51              
+ Partials        7        6       -1

Impacted Files	Coverage Δ
src/lib/themes/dark_theme.ts	`100% <ø> (ø)`	⬆️
src/lib/themes/theme.ts	`100% <ø> (ø)`	⬆️
src/lib/series/specs.ts	`100% <ø> (ø)`	⬆️
src/lib/themes/light_theme.ts	`100% <ø> (ø)`	⬆️
src/state/utils.ts	`97.5% <100%> (+1.06%)`	⬆️
...onents/react_canvas/utils/rendering_props_utils.ts	`100% <100%> (ø)`	⬆️
src/lib/series/rendering.ts	`99.27% <100%> (+1.35%)`	⬆️
src/lib/series/series.ts	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e727605...acb5a91. Read the comment docs.

src/components/react_canvas/bar_geometries.tsx

src/components/react_canvas/line_geometries.tsx

emmacunningham

tested locally on firefox and code LGTM just a small couple of questions

emmacunningham · 2019-07-05T12:11:02Z

src/lib/themes/theme.ts

+  rect: RectStyle;
+  rectBorder: RectBorderStyle;
+  displayValue: DisplayValueStyle;
+}


For some of the properties above, we have defined interfaces for them already. Should we use them where possible? For example, we have an Opacity interface that could be referenced instead of duplicating it manually within RectStyle. (The same for FillStyle, StrokeStyle, Visible and StrokeDashArray.) If we aren't going to reuse those interfaces, should we delete them? Looks like Radius was already deleted in favor of defining it inside the interface.

@emmacunningham here my difficulty was the following: the StrokeStyle is specified as

export interface StrokeStyle { /** The stroke color in hex, rgba, hsl */ stroke: string; /** The stroke width in pixel */ strokeWidth: number; }

but I want to to have the stroke optional and the strokeWidth required. Is there a way to achieve that? I can change the StrokeStyle type but we maybe need a different configuration in the future. Splitting the StrokeStyle into two types is useless and we can just go removing all the Stroke/Opacity interfaces.
What do you think?

I believe you can do something like this to extend StrokeStyle such that stroke becomes optional and strokeWidth remains required.

First, we need a way to keep one property as required and make the rest optional:

export type RequireOneAndMakeRestOptional<T, K extends keyof T> = { [X in Exclude<keyof T, K>]?: T[X] } & { [P in K]-?: T[P] };

and then RectBorderStyle for example could be:

export type RectBorderStyle = RequireOneAndMakeRestOptional<StrokeStyle, 'strokeWidth'> & Visible;

I'm not sure this is that much better since it requires other contributors to understand what RequireOneAndMakeRestOptional is doing to understand the type annotations where it's being used. I think that the way you have it now is more easy to follow and read.

Having the interfaces for these common styling properties I think is useful because it makes it easier for other contributors to compose new interfaces that are consistent (so we don't accidentally type stroke as a number sometimes, for example). In particulary, I think StrokeDashArray is especially useful because it is not obvious what this should be (in fact, we could probaably make this more explicit as [number, number]) for someone not familiar with the API to create a dashed stroke.

I will leave this as it is for the moment, and we can find a clear type later. You are completely right about the fact that a common styling type is required, I will like to open a PR with that in the next future #264

stories/bar_chart.tsx

nickofthyme

Tested locally, LGTM. Only a few comments around mergeWithDefaultTheme.

stories/bar_chart.tsx

stories/styling.tsx

# [8.0.0](v7.2.1...v8.0.0) (2019-07-15) ### Code Refactoring * **legend:** remove visibility button ([#252](#252)) ([90a1ba7](90a1ba7)) ### Features * **style:** allow fill and stroke overrides ([#258](#258)) ([99c5e9f](99c5e9f)) ### BREAKING CHANGES * **style:** `LineStyle`, `AreaStyle` and `BarSeriesStyle` types differs on the optional values. `stroke` and `fill` on the theme or specific series style now override the computed series color. * fix: no unused locals error on theme * fix: avoid key prop override by spread operator * test: increase test coverage on PR changes * fix: fontSize is now always a number * test: increase coverage for rendendering * refactor(story): simplify theme merging on `with value label` story * refactor: removed mergeWithDefaultTheme * **legend:** the `onLegendItemClick` click handler is no longer applied when clicking on the title. Instead a simple visibility change is applied.

markov00 · 2019-07-15T10:16:28Z

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [8.0.0](elastic/elastic-charts@v7.2.1...v8.0.0) (2019-07-15) ### Code Refactoring * **legend:** remove visibility button ([opensearch-project#252](elastic/elastic-charts#252)) ([efb3823](elastic/elastic-charts@efb3823)) ### Features * **style:** allow fill and stroke overrides ([opensearch-project#258](elastic/elastic-charts#258)) ([1648996](elastic/elastic-charts@1648996)) ### BREAKING CHANGES * **style:** `LineStyle`, `AreaStyle` and `BarSeriesStyle` types differs on the optional values. `stroke` and `fill` on the theme or specific series style now override the computed series color. * fix: no unused locals error on theme * fix: avoid key prop override by spread operator * test: increase test coverage on PR changes * fix: fontSize is now always a number * test: increase coverage for rendendering * refactor(story): simplify theme merging on `with value label` story * refactor: removed mergeWithDefaultTheme * **legend:** the `onLegendItemClick` click handler is no longer applied when clicking on the title. Instead a simple visibility change is applied.

markov00 requested review from emmacunningham and nickofthyme July 3, 2019 13:32

markov00 added :styling Styling related issue enhancement New feature or request labels Jul 3, 2019

fix: no unused locals error on theme

bc54461

nickofthyme reviewed Jul 3, 2019

View reviewed changes

src/components/react_canvas/bar_geometries.tsx Outdated Show resolved Hide resolved

nickofthyme reviewed Jul 3, 2019

View reviewed changes

src/components/react_canvas/line_geometries.tsx Outdated Show resolved Hide resolved

markov00 added 5 commits July 3, 2019 18:45

fix: avoid key prop override by spread operator

111a465

test: increase test coverage on PR changes

015e053

Merge branch 'master' into add_fill_point_style

60f8b4c

fix: fontSize is now always a number

f1bf31b

test: increase coverage for rendendering

9d43337

markov00 requested a review from nickofthyme July 4, 2019 17:03

emmacunningham approved these changes Jul 5, 2019

View reviewed changes

markov00 added 2 commits July 5, 2019 15:41

refactor(story): simplify theme merging on with value label story

274cf29

Merge branch 'master' into add_fill_point_style

cbd23ad

nickofthyme approved these changes Jul 11, 2019

View reviewed changes

stories/bar_chart.tsx Outdated Show resolved Hide resolved

stories/styling.tsx Outdated Show resolved Hide resolved

stories/styling.tsx Outdated Show resolved Hide resolved

stories/styling.tsx Outdated Show resolved Hide resolved

refactor: removed mergeWithDefaultTheme

acb5a91

markov00 mentioned this pull request Jul 15, 2019

Common styling types #264

Closed

3 tasks

markov00 merged commit 99c5e9f into elastic:master Jul 15, 2019

markov00 added the released Issue released publicly label Jul 15, 2019

markov00 mentioned this pull request Jul 31, 2019

fix(theme): restore original point radius values #276

Merged

4 tasks

markov00 mentioned this pull request Aug 16, 2019

docs: update custom fill/stroke styles on storybook #318

Merged

2 tasks

markov00 deleted the add_fill_point_style branch November 25, 2020 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(style): allow fill and stroke overrides #258

feat(style): allow fill and stroke overrides #258

markov00 commented Jul 3, 2019 •

edited

Loading

codecov-io commented Jul 3, 2019 •

edited

Loading

emmacunningham left a comment

emmacunningham Jul 5, 2019

markov00 Jul 5, 2019

emmacunningham Jul 8, 2019

markov00 Jul 15, 2019

nickofthyme left a comment

markov00 commented Jul 15, 2019

feat(style): allow fill and stroke overrides #258

feat(style): allow fill and stroke overrides #258

Conversation

markov00 commented Jul 3, 2019 • edited Loading

Summary

Checklist

codecov-io commented Jul 3, 2019 • edited Loading

Codecov Report

emmacunningham left a comment

Choose a reason for hiding this comment

emmacunningham Jul 5, 2019

Choose a reason for hiding this comment

markov00 Jul 5, 2019

Choose a reason for hiding this comment

emmacunningham Jul 8, 2019

Choose a reason for hiding this comment

markov00 Jul 15, 2019

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

markov00 commented Jul 15, 2019

markov00 commented Jul 3, 2019 •

edited

Loading

codecov-io commented Jul 3, 2019 •

edited

Loading