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

[SIP-5] Refactor and improve histogram #5758

Merged
merged 11 commits into from Sep 5, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Aug 28, 2018

  • Replace original histogram code with @data-ui/histogram
  • Use @vx/legend for legend
  • Separate the component from slice and formData

Before
image

After
image

@williaster @conglei

@codecov-io
Copy link

codecov-io commented Aug 28, 2018

Codecov Report

Merging #5758 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5758      +/-   ##
=========================================
- Coverage   63.83%   63.8%   -0.04%     
=========================================
  Files         364     365       +1     
  Lines       23100   23111      +11     
  Branches     2587    2600      +13     
=========================================
  Hits        14745   14745              
- Misses       8340    8351      +11     
  Partials       15      15
Impacted Files Coverage Δ
superset/assets/src/visualizations/sunburst.js 0% <ø> (ø) ⬆️
superset/assets/src/visualizations/WithLegend.jsx 0% <0%> (ø)
superset/assets/src/visualizations/Histogram.jsx 0% <0%> (ø)
...sualizations/deckgl/CategoricalDeckGLContainer.jsx 0% <0%> (ø) ⬆️
superset/assets/src/visualizations/cal_heatmap.js 0% <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 0c33f80...9c3990e. Read the comment docs.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM!!! excited this seems to have worked smoothly, still curious how it sizes correctly with the legend 🍀

<LegendOrdinal
scale={colorScale}
direction="row"
shape="rect"
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see rect being better than circles given that bars are rectangle. If you do want them to render as rects, you can do something like (example here / storybook).

tldr

shape={({ fill, width, height }) => (
  <svg width={width} height={height}>
    <rect width={width} height={height} fill={fill} />
  </svg>
)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah. nice.

@kristw
Copy link
Contributor Author

kristw commented Aug 29, 2018

Let me hold this one a bit. May wrap the legend and chart in flexbox to make sure. I feel that magic layout is too good to be true.

@kristw
Copy link
Contributor Author

kristw commented Aug 30, 2018

I write a layout component called WithLegend

  • Given renderLegend and renderChart as render props, it will give space enough to render the legend and give the rest of the container to the chart (via flexbox).
  • renderChart is wrapped in @vx/responsive/ParentSize and will be given parent. Developer then can pass parent.width and parent.height to the chart.
  • Developer can also set the legend position (top, left, bottom, right), and it will put the legend there.
  • It will also pass the flexDirection to the legend component, which @vx/legend can directly use to arrange item left-to-right or top-to-bottom appropriately. For example, legend on the top should arrange legend items left-to-right, legend on the right should arrange legend items top-to-bottom

p.s. The shape="rect" gives rect, but my screenshot above was outdated. Thanks for the custom shape tldr tho. Very useful.

@kristw
Copy link
Contributor Author

kristw commented Aug 30, 2018

position="top"
image

position="left"
image

position="right"
image

position="bottom"
image

@williaster
Copy link
Contributor

williaster commented Sep 5, 2018

this lgtm, I like the WithLegend component, it seems like it should be pretty flexible ( ;) ) with the various render props, too.

My only comment would be on aesthetics for the bottom orientation: should we match the flex-end justify-content alignment of the top orientation for consistency instead of center? or maybe flex-start for bottom? could get @elibrumbaugh 's thoughts on this as well, or I'm happy merging this to land it and making tweaks later.

@williaster williaster merged commit b461287 into apache:master Sep 5, 2018
@kristw kristw deleted the kristw-histogram branch September 5, 2018 21:06
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
* Extract slice and formData

* indent

* update data proptype

* enable theme

* remove legacy code

* rename file

* Add legend

* Implement WithLegend

* align legend items to the right for bottom position

* add line at end of file

* fix linting issues
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Extract slice and formData

* indent

* update data proptype

* enable theme

* remove legacy code

* rename file

* Add legend

* Implement WithLegend

* align legend items to the right for bottom position

* add line at end of file

* fix linting issues
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants