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: allow individual series styling #170

Merged
merged 10 commits into from
Apr 16, 2019

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented Apr 12, 2019

Summary

close #138

This PR introduces the ability to customize the styles for a series independently from the main chart theme (which is shared across all series). If both a custom individual series style and custom chart theme are defined, precedence will be given to the custom series style (though the chart theme can still be used to change styles for the series which do not have their own custom series styles).

Currently, this supports the same set of customizable styling options as the main chart theme, but can be extended to cover additional styles currently not implemented as customizable.

The following examples can be found under Styling in storybook:

Bars

currently supported customizable options:

  • border stroke width
  • border stroke color
  • border visibility
  • bars opacity (currrently not configurable from chart theme, but seems something that TSVB allows as a series-specific customization)

bar_style

Lines

currently supported customizable options:

  • line stroke width
  • line visibility
  • line opacity
  • point visibility
  • point radius
  • point opacity

line_style

Areas

currently supported customizable options:

  • line stroke width
  • line visibility
  • line opacity
  • point visibility
  • point radius
  • point opacity
  • area visibility
  • area opacity

area_style

Checklist

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

  • 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

@emmacunningham emmacunningham added enhancement New feature or request wip work in progress :data Data/series/scales related issue labels Apr 12, 2019
@codecov-io
Copy link

Codecov Report

Merging #170 into master will increase coverage by 0.35%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
+ Coverage   96.26%   96.62%   +0.35%     
==========================================
  Files          36       36              
  Lines        1849     1867      +18     
  Branches      239      253      +14     
==========================================
+ Hits         1780     1804      +24     
+ Misses         60       54       -6     
  Partials        9        9
Impacted Files Coverage Δ
src/lib/themes/theme.ts 100% <ø> (ø) ⬆️
src/lib/series/specs.ts 100% <ø> (ø) ⬆️
src/state/utils.ts 91.37% <100%> (+0.15%) ⬆️
src/lib/series/rendering.ts 96.11% <100%> (+6.53%) ⬆️
...onents/react_canvas/utils/rendering_props_utils.ts 100% <100%> (ø) ⬆️

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 c53dc4d...a38db49. Read the comment docs.

@@ -90,16 +90,24 @@ export interface Theme {
export interface BarSeriesStyle {
border: StrokeStyle & Visible;
}

export type CustomBarSeriesStyle = BarSeriesStyle & Partial<Opacity>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was created because BarSeriesStyle currently does not have a way to control opacity of the bar elements itself outside of the hover-effects; if we want to change this and add opacity as a property directly on the BarSeriesStyle interface, I can get rid of this extra type.

@emmacunningham emmacunningham removed the wip work in progress label Apr 12, 2019
@emmacunningham emmacunningham requested a review from markov00 April 12, 2019 17:48
@markov00 markov00 mentioned this pull request Apr 12, 2019
93 tasks
@emmacunningham emmacunningham added :chart Chart element related issue and removed :data Data/series/scales related issue labels Apr 12, 2019
@@ -38,6 +46,7 @@ export interface PointGeometry {
};
geometryId: GeometryId;
value: GeometryValue;
seriesPointStyle?: PointStyle;
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't have individual style for each point, I think we can move this to the AreaGeometry and LineGeometry. Will avoid having this style copied to all data point and we can avoid checking for visibility on each datapoint on a line (we can just check it in the first series cycle. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved seriesPointStyle to AreaGeometry and LineGeometry. also moved the series-related style computation to when we iterate over each line/area (before this was happening for every point, even though they will all have the same styles (strokeWidth, radius, opacity) per line/area).

see f465807.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally

@emmacunningham emmacunningham merged commit c780d98 into elastic:master Apr 16, 2019
markov00 pushed a commit that referenced this pull request Apr 16, 2019
# [3.11.0](v3.10.2...v3.11.0) (2019-04-16)

### Bug Fixes

* remove old specs with changed ids ([#167](#167)) ([6c4f705](6c4f705))

### Features

* allow individual series styling ([#170](#170)) ([c780d98](c780d98))
@markov00
Copy link
Member

🎉 This PR is included in version 3.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 16, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:chart Chart element related issue enhancement New feature or request released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to style each series
3 participants