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(series): set custom series colors through spec prop #95

Merged
merged 5 commits into from
Mar 11, 2019

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented Mar 8, 2019

Summary

close #39

This PR adds the ability to specify a custom series colors through a prop on the series spec.

Each series can now receive an optional prop customSeriesColor: Map<DataSeriesColorValues, string>, where the key is used to identify the series and the string is a representation of the color. Under the Stylings section of Storyboard, there is an example of this in action.

custom_series_colors

The color picker in the legend is also still usable to customize a series color; on color picker update, the customSeriesColor prop for the corresponding series is updated.

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 wip work in progress :data Data/series/scales related issue :specs Chart specifications related issue :legend Legend related issue and removed :data Data/series/scales related issue labels Mar 8, 2019
@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #95 into master will increase coverage by 0.56%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   88.07%   88.63%   +0.56%     
==========================================
  Files          28       28              
  Lines        1132     1144      +12     
  Branches      118      118              
==========================================
+ Hits          997     1014      +17     
+ Misses        125      120       -5     
  Partials       10       10
Impacted Files Coverage Δ
src/lib/series/specs.ts 100% <ø> (ø) ⬆️
src/state/utils.ts 72.72% <100%> (+1.93%) ⬆️
src/lib/series/series.ts 92.96% <100%> (-0.11%) ⬇️
src/state/chart_state.ts 98.94% <100%> (+0.03%) ⬆️
src/lib/axes/canvas_text_bbox_calculator.ts 100% <0%> (+31.25%) ⬆️

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 dcee68e...2ab4a66. Read the comment docs.

@emmacunningham emmacunningham removed the wip work in progress label Mar 8, 2019
@emmacunningham emmacunningham requested a review from markov00 March 8, 2019 21:00
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 and works fine
One minor thing to notice: if, in the future we want to to have a sort of JSON schema for the spec, how do you think we can serialize Map<DataSeriesColorsValues, string>

@markov00 markov00 mentioned this pull request Mar 8, 2019
93 tasks
@emmacunningham
Copy link
Contributor Author

@markov00 for serializing the Map<DataSeriesColorsValues, string> prop: there are a few options I've thought about. The current implementation has the advantage that it makes it easier for the downstream user to define than need to know more specifics about the internals of the library. But if we want to have a JSON schema for the spec, then the user could define an Array of:

{
  dataSeriesColorValues: DataSeriesColorValues;
  color: string
}

This still allows the user to define in terms of DataSeriesColorValues (instead of needing to know more about the chart internals and forming the string series key themselves) while providing a bit of type safety to how they are defining these so that we at least get both a specId and colorValues to uniquely identify the series. We would need to then change the way that updating the props works; because currently it is a map, we can just set the customSeriesColors map using the key when there is an update. Using this new data structure, we'd need to iterate through the array for the spec and then find if a config with a matching DataSeriesColorValues exists.

Another option would be to have this prop be an object (one advantage is that the updating logic would remain about the same, unlike the option above):

{
  [seriesKey: string]: string;
}

where the seriesKey property is the stringified form of the DataSeriesColorValues and the value is string representing color. The drawback to this is that the user would need to be able to form the seriesKey, though we do have a function that they could call on DataSeriesColorValues to generate this string.

@emmacunningham emmacunningham merged commit fb09dc9 into elastic:master Mar 11, 2019
markov00 pushed a commit that referenced this pull request Mar 11, 2019
# [3.1.0](v3.0.1...v3.1.0) (2019-03-11)

### Features

* **series:** set custom series colors through spec prop ([#95](#95)) ([fb09dc9](fb09dc9))
@markov00
Copy link
Member

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Mar 11, 2019
@markov00 markov00 modified the milestone: 7.1 Mar 26, 2019
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [3.1.0](elastic/elastic-charts@v3.0.1...v3.1.0) (2019-03-11)

### Features

* **series:** set custom series colors through spec prop ([opensearch-project#95](elastic/elastic-charts#95)) ([581c97e](elastic/elastic-charts@581c97e))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:legend Legend related issue released Issue released publicly :specs Chart specifications related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add external custom color from spec props
3 participants