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: add point shapes #988

Merged
merged 17 commits into from
Jan 29, 2021

Conversation

markov00
Copy link
Member

Summary

This PR adds the ability to choose the shape of the rendered point as per request for Timelion replacement.
Screenshot 2021-01-20 at 18 28 37

fix #987

Checklist

Delete any items that are not 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)
  • 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

@markov00 markov00 added enhancement New feature or request :chart Chart element related issue :xy Bar/Line/Area chart related labels Jan 20, 2021
@codecov-io
Copy link

codecov-io commented Jan 20, 2021

Codecov Report

Merging #988 (9a89f3e) into master (d78d1ce) will increase coverage by 0.27%.
The diff coverage is 73.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
+ Coverage   72.63%   72.90%   +0.27%     
==========================================
  Files         350      369      +19     
  Lines       11058    11362     +304     
  Branches     2433     2467      +34     
==========================================
+ Hits         8032     8284     +252     
- Misses       3012     3056      +44     
- Partials       14       22       +8     
Flag Coverage Δ
unittests 72.49% <72.44%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/chart_types/xy_chart/renderer/canvas/areas.ts 30.00% <0.00%> (ø)
src/chart_types/xy_chart/rendering/area.ts 91.66% <ø> (ø)
src/chart_types/xy_chart/rendering/bubble.ts 100.00% <ø> (ø)
src/chart_types/xy_chart/rendering/line.ts 95.00% <ø> (ø)
src/chart_types/xy_chart/utils/interactions.ts 100.00% <ø> (ø)
src/utils/geometry.ts 80.00% <ø> (-20.00%) ⬇️
.../chart_types/xy_chart/renderer/dom/highlighter.tsx 56.52% <15.78%> (-8.35%) ⬇️
src/chart_types/xy_chart/renderer/canvas/points.ts 51.85% <55.55%> (-2.00%) ⬇️
src/chart_types/xy_chart/renderer/shapes_paths.ts 58.33% <58.33%> (ø)
src/chart_types/xy_chart/rendering/points.ts 89.58% <80.00%> (+0.33%) ⬆️
... and 36 more

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 d78d1ce...f04a536. Read the comment docs.

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM - tested locally on my mac. I like how this is set up to easily add other point shapes in the future in the theme. Defaulting to circle makes a lot of sense. 👍

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder how we could change the highlighter in the future based on the point shape type so we don't get this...

Screen Recording 2021-01-21 at 10 22 AM

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@markov00
Copy link
Member Author

LGTM. I wonder how we could change the highlighter in the future based on the point shape type so we don't get this...

Screen Recording 2021-01-21 at 10 22 AM

@nickofthyme @monfera @rshen91 I've updated the PR after Nick's comment here.
I've matched the highlighter shape with the point shape.
I've also cleaned up the canvas rendering functions and created a set of reusable SVG paths.
We have now the ability to display also bubble charts with different shape sizes like:
Screenshot 2021-01-27 at 10 25 35
or having a single series with customized shapes per point:
Screenshot 2021-01-27 at 10 31 10

I've also introduced a change on the current PointGeometry, computing the style override at the same time when computing the point position and scales, instead of computing it in the renderer.

@monfera
Copy link
Contributor

monfera commented Jan 27, 2021

With the broad generalization introduced with the last commits, we, at some point, might want to move away from singleton legends, because each aesthetic channel may encode different categorical or quantitative data. Quick example:
image

(the size legend is rendered somewhat useless though, due to lack of decent comparability of sizes)

While the marker shape may be redundant encoding for line series, eg. for accessibility and discernibility in print (at the cost of making the lines noisier), scatterplots and connected scatterplots would be more inviting for independent channel assignment.

So with that, the question is, what are the tradeoffs of introducing broad configurability of markers as an independent categorical-only channel with or without independent legends. Without this, the ability to vary markers may not be super useful, though users can just avoid configuring it.

Edit: if we model marker shapes as redundant encoding that (currently) needs no extra legend, because it's always in sync with the color, then maybe the legend swatch could reflect the marker shapes. ggplot2 examples for independent vs redundant(combined):

image

Circle: 'circle' as const,
Square: 'square' as const,
Diamond: 'diamond' as const,
Cross: 'cross' as const,
Copy link
Contributor

@monfera monfera Jan 27, 2021

Choose a reason for hiding this comment

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

An oft used name is, eg. listed in ggplot2, for +: plus; for x: cross.

Matplotlib also uses plus for the upright one, but uses x for x - the list is also a good bit broader, eg. there's a "filled plus" version. Might eventually be useful, eg. if the purpose is to lend each marker a comparable visual weight. There seem to be full bodied shapes, stroke based shapes and outline based shapes.

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Good functional addition; asking for renaming "angled cross" to something standard, eg. cross, while renaming current cross to plus. Also, a couple of follow-up issues are warranted:

  • linking back to the independent legend point with examples
  • using, or making it configurable, the markers in the legend, if redundant encoding is chosen

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM. Nice job with the highlighter shapes, that looks much better!

I wonder if in the future we allow the user to pass a path, if we could scale that up and take the outline silhouette of the shape as the highlight geometry rather than just hardcoding each outlined shape. Do you think that would be possible for most shapes?

@markov00 markov00 merged commit 1392b7d into elastic:master Jan 29, 2021
github-actions bot pushed a commit that referenced this pull request Jan 30, 2021
# [24.5.0](v24.4.0...v24.5.0) (2021-01-30)

### Bug Fixes

* add theme min radius to point shape ([#996](#996)) ([eb37175](eb37175))
* align tooltip z-index to EUI tooltip z-index ([#931](#931)) ([ffd626b](ffd626b))
* chart state and series functions cleanup ([#989](#989)) ([944ac6c](944ac6c))
* create unique ids for dot icons ([#971](#971)) ([e1ce768](e1ce768))
* external tooltip legend extra value sync ([#993](#993)) ([13ad05a](13ad05a))
* **legend:** disable focus and keyboard navigation for legend in partition ch… ([#952](#952)) ([03bd2f7](03bd2f7))
* **legend:** hierarchical legend order should follow the tree paths ([#947](#947)) ([f9218ad](f9218ad)), closes [#944](#944)
* **legend:** remove ids for circles ([#973](#973)) ([b3f4f90](b3f4f90))

### Features

* **cursor:** improve theme styling for crosshair ([#980](#980)) ([6c4dafd](6c4dafd))
* **legend:**  display pie chart legend extra ([#939](#939)) ([d14de01](d14de01))
* **legend:** add keyboard navigation ([#880](#880)) ([87c227d](87c227d))
* **partition:** Flame and icicle chart ([#965](#965)) ([3df73d0](3df73d0))
* **partition:** legend hover options ([#978](#978)) ([f810d94](f810d94))
* **xy:** support multiple point shapes on line, area and bubble charts ([#988](#988)) ([1392b7d](1392b7d))
@markov00
Copy link
Member Author

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [24.5.0](elastic/elastic-charts@v24.4.0...v24.5.0) (2021-01-30)

### Bug Fixes

* add theme min radius to point shape ([opensearch-project#996](elastic/elastic-charts#996)) ([98089a9](elastic/elastic-charts@98089a9))
* align tooltip z-index to EUI tooltip z-index ([opensearch-project#931](elastic/elastic-charts#931)) ([f7f1f6f](elastic/elastic-charts@f7f1f6f))
* chart state and series functions cleanup ([opensearch-project#989](elastic/elastic-charts#989)) ([42a7af0](elastic/elastic-charts@42a7af0))
* create unique ids for dot icons ([opensearch-project#971](elastic/elastic-charts#971)) ([0b3e00f](elastic/elastic-charts@0b3e00f))
* external tooltip legend extra value sync ([opensearch-project#993](elastic/elastic-charts#993)) ([7e1096e](elastic/elastic-charts@7e1096e))
* **legend:** disable focus and keyboard navigation for legend in partition ch… ([opensearch-project#952](elastic/elastic-charts#952)) ([dfff3e2](elastic/elastic-charts@dfff3e2))
* **legend:** hierarchical legend order should follow the tree paths ([opensearch-project#947](elastic/elastic-charts#947)) ([7b70186](elastic/elastic-charts@7b70186)), closes [opensearch-project#944](elastic/elastic-charts#944)
* **legend:** remove ids for circles ([opensearch-project#973](elastic/elastic-charts#973)) ([ed98481](elastic/elastic-charts@ed98481))

### Features

* **cursor:** improve theme styling for crosshair ([opensearch-project#980](elastic/elastic-charts#980)) ([0248ad6](elastic/elastic-charts@0248ad6))
* **legend:**  display pie chart legend extra ([opensearch-project#939](elastic/elastic-charts#939)) ([672a4df](elastic/elastic-charts@672a4df))
* **legend:** add keyboard navigation ([opensearch-project#880](elastic/elastic-charts#880)) ([b471a94](elastic/elastic-charts@b471a94))
* **partition:** Flame and icicle chart ([opensearch-project#965](elastic/elastic-charts#965)) ([9e8b1f7](elastic/elastic-charts@9e8b1f7))
* **partition:** legend hover options ([opensearch-project#978](elastic/elastic-charts#978)) ([acd1339](elastic/elastic-charts@acd1339))
* **xy:** support multiple point shapes on line, area and bubble charts ([opensearch-project#988](elastic/elastic-charts#988)) ([4f23b4f](elastic/elastic-charts@4f23b4f))
@markov00 markov00 deleted the 2021_01_20-feat_add_point_shapes branch April 11, 2022 10:39
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 :xy Bar/Line/Area chart related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable different shapes for markers
5 participants