Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

fix: single y axis bounds #148

Merged
merged 1 commit into from
Jul 25, 2019
Merged

Conversation

etr2460
Copy link
Contributor

@etr2460 etr2460 commented Jul 24, 2019

🐛 Bug Fix

Further fixes to the custom y axis bounds for nvd3 charts. We were only respecting custom y axis bounds if they were both set; this allows the user to only set 1 custom y axis bound. The other one will be automatically calculated based on what type of chart this is and what the max/min data values are.

Note that this also special cases expanded area and stream area charts from having custom y axis bounds. These charts don't make much sense without the default bounds, so I've disabled them.

Screenshots:
Screen Shot 2019-07-24 at 4 36 43 PM
Screen Shot 2019-07-24 at 4 36 52 PM
bounds 0 - 3 billion
Screen Shot 2019-07-24 at 4 37 15 PM

bounds 1 billion - null
Screen Shot 2019-07-24 at 4 37 24 PM
Screen Shot 2019-07-24 at 4 37 31 PM

ignore custom bounds
Screen Shot 2019-07-24 at 4 37 38 PM

bar charts without bounds still work
Screen Shot 2019-07-24 at 4 37 47 PM
Screen Shot 2019-07-24 at 4 37 53 PM

reviewers: @khtruong @kristw @williaster @graceguo-supercat

@etr2460 etr2460 requested a review from a team as a code owner July 24, 2019 23:51
@etr2460 etr2460 force-pushed the erik-ritter--fix-single-y-bounds branch from 38745e9 to 7e45ee6 Compare July 25, 2019 17:44
@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #148 into master will increase coverage by 6.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #148      +/-   ##
=========================================
+ Coverage   19.87%   25.9%   +6.02%     
=========================================
  Files           8       8              
  Lines         161     166       +5     
  Branches       10      10              
=========================================
+ Hits           32      43      +11     
+ Misses        128     122       -6     
  Partials        1       1
Impacted Files Coverage Δ
.../superset-ui-legacy-preset-chart-nvd3/src/utils.js 14.81% <100%> (+7.89%) ⬆️

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 d770a14...39e8a2e. 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.

This LGTM. I agree about disabling custom bounds for expanded (0-100%) area charts, and I also agree with using the domain for stream area charts 👍

chart.style() === 'stream'
) {
// Because there are custom bounds, we need to override them back to the domain of the
// chart since this is a stream area chart
Copy link
Contributor

Choose a reason for hiding this comment

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

domain of the data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

applyYAxisBounds();

// Also reapply on each state change to account for enabled/disabled series
chart.dispatch.on('stateChange.applyYAxisBounds', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove the extra arrow func here?

chart.dispatch.on('stateChange.applyYAxisBounds', applyYAxisBounds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, done!

@etr2460 etr2460 force-pushed the erik-ritter--fix-single-y-bounds branch from 7e45ee6 to addb096 Compare July 25, 2019 18:38
@etr2460 etr2460 force-pushed the erik-ritter--fix-single-y-bounds branch from addb096 to 39e8a2e Compare July 25, 2019 19:00
@etr2460
Copy link
Contributor Author

etr2460 commented Jul 25, 2019

tests added too

@williaster
Copy link
Contributor

🔥

@etr2460 etr2460 merged commit 49da856 into master Jul 25, 2019
@delete-merged-branch delete-merged-branch bot deleted the erik-ritter--fix-single-y-bounds branch July 25, 2019 19:56
nytai pushed a commit to preset-io/superset-ui-plugins that referenced this pull request Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants