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): markSizeAccessor and BubbleSeries (alpha) #559

Merged
merged 60 commits into from
Apr 21, 2020

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Feb 19, 2020

Summary

Closes #533

Allow for a markSizeAccessor to render as radius size for point values. Copied Kibana logic for dotRadio using a step function, but this could be replaced with our own continuous sizing function.

Add BubbleSeries spec. This spec only draws points with no lines. The unique aspect of this type is that it uses the d3-delaunay spatial indexing approach. Currently, this does not work well with other chart spec types as the tooltip will show all values in range even on different x values. This is NOT a scatter plot, this type will come in another PR in the future.

Tasks:

Bubble

Screen Recording 2020-03-31 at 11 17 AM

Area

Screen Recording 2020-03-31 at 11 15 AM

Line

Screen Recording 2020-03-31 at 11 16 AM

Checklist

  • 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

@nickofthyme nickofthyme requested a review from markov00 February 19, 2020 18:28
@wylieconlon
Copy link

This looks like a first step towards a real scatter chart, right? #502

@wylieconlon
Copy link

Instead of calling this dot, can you call it something like markSize? The mark in this case is a dot, but like I requested in the scatter chart issue it would be nice to support other shapes.

@nickofthyme nickofthyme changed the title Add bubble chart (aka dot accessor) feat(series): dot accessor Mar 31, 2020
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.

I've done a quick check through the PR code and added few minor comments

I've found some weird issues when moving my mouse over multiple points
Seems that in some cases the number of datapoints in the tooltip is different from the number of elements highlighted (I can spot some duplicated values, like the bubbles 80 in this example)
Apr-01-2020 12-37-27

src/scales/scale_band.ts Outdated Show resolved Hide resolved
src/specs/settings.tsx Outdated Show resolved Hide resolved
src/utils/geometry.ts Outdated Show resolved Hide resolved
src/utils/themes/theme.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/indexed_geometry_map.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/indexed_geometry_map.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/indexed_geometry_map.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/series.ts Outdated Show resolved Hide resolved
src/chart_types/xy_chart/utils/specs.ts Outdated Show resolved Hide resolved
- Cleanup yDomain tests with mocks
- Cleanup stackedPercentSeries tests with mocks
- Add isDefined and isDefinedFrom with tests
- Make mark prop on RawDatum optional
- Require datum on DataSeriesDatum
- Update series mocks
- Add rawDataSeries mock from data or data[]
- Fix issue with null values in mergePartial
- Update mergePartial logic to treat Maps like Objects
- Update mergePartial logic to treat Sets like Arrays
- Add tests for new functionality
- Add Geometry mocks
- Update Rendering bands tests with mocks
- Fix RecursivePartial for nested any, any[], Set and Map
- Add option to mergeMaps, default is to replace.
- point tests
- rendering area tests
- rendering bubble tests
- rendering line tests
- general rendering tests
- band and continuous scale tests
@nickofthyme nickofthyme requested a review from markov00 April 16, 2020 23:07
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, great work on that!
Tested locally and works fine. I think we should address in the near future adding a more descriptive tooltip because is not very clear to me, what is X, Y, and radius value

@nickofthyme
Copy link
Collaborator Author

@markov00 I agree, we really need to update the tooltip to account for more cases.

@nickofthyme nickofthyme merged commit 3aa235e into elastic:master Apr 21, 2020
@nickofthyme nickofthyme deleted the feat/dot-accessor branch April 21, 2020 17:24
markov00 pushed a commit that referenced this pull request Apr 22, 2020
# [18.4.0](v18.3.0...v18.4.0) (2020-04-22)

### Bug Fixes

* **partition:** single slice wrong text positioning ([#643](#643)) ([6298d36](6298d36)), closes [#637](#637)
* **treemap:** align onElementClick parameters to sunburst ([#636](#636)) ([2c1d224](2c1d224)), closes [#624](#624)

### Features

* allow colorVariant option for series specific color styles ([#630](#630)) ([e5a206d](e5a206d))
* **series:** BubbleSeries (alpha) and markSizeAccessor ([#559](#559)) ([3aa235e](3aa235e))
@markov00
Copy link
Member

🎉 This PR is included in version 18.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Apr 22, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [18.4.0](elastic/elastic-charts@v18.3.0...v18.4.0) (2020-04-22)

### Bug Fixes

* **partition:** single slice wrong text positioning ([opensearch-project#643](elastic/elastic-charts#643)) ([f8b5b8a](elastic/elastic-charts@f8b5b8a)), closes [opensearch-project#637](elastic/elastic-charts#637)
* **treemap:** align onElementClick parameters to sunburst ([opensearch-project#636](elastic/elastic-charts#636)) ([8dd87bf](elastic/elastic-charts@8dd87bf)), closes [opensearch-project#624](elastic/elastic-charts#624)

### Features

* allow colorVariant option for series specific color styles ([opensearch-project#630](elastic/elastic-charts#630)) ([e2444ef](elastic/elastic-charts@e2444ef))
* **series:** BubbleSeries (alpha) and markSizeAccessor ([opensearch-project#559](elastic/elastic-charts#559)) ([85d9bda](elastic/elastic-charts@85d9bda))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly :vislib Relating to vislib replacement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dot size based on point data
4 participants