-
Notifications
You must be signed in to change notification settings - Fork 120
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
fix(domain): set domain bounds dependent on negative/positive values #149
fix(domain): set domain bounds dependent on negative/positive values #149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
==========================================
+ Coverage 94.03% 94.06% +0.03%
==========================================
Files 34 34
Lines 1744 1753 +9
Branches 219 223 +4
==========================================
+ Hits 1640 1649 +9
Misses 90 90
Partials 14 14
Continue to review full report at Codecov.
|
} | ||
|
||
// if any of the values are null | ||
return [0, 0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d3.extent()
has the type signature [number, number] | [undefined, undefined]
, so there's a possibility that the computed range start and end could be undefined
, though consumers of this function appear to operate under the assumption that this is always [number, number]
.
This seems like a case where if any value within the computed extent is undefined
, we should handle it as an error and surface it to the user somehow (see #117). The other option is that we coerce the undefined
s:
[ -start, undefined ] => [ -start, 0 ]
[ +start, undefined ] => [ 0, start ] ?
[ undefined, -end ] => [ end, 0 ] ?
[ undefined, +end ] => [ 0, end ]
[ undefined, undefined ] => [0, 0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM!
## [3.7.1](v3.7.0...v3.7.1) (2019-04-05) ### Bug Fixes * **domain:** set domain bounds dependent on negative/positive values ([#149](#149)) ([5b16be6](5b16be6))
🎉 This PR is included in version 3.7.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [3.7.1](elastic/elastic-charts@v3.7.0...v3.7.1) (2019-04-05) ### Bug Fixes * **domain:** set domain bounds dependent on negative/positive values ([opensearch-project#149](elastic/elastic-charts#149)) ([1e9d96d](elastic/elastic-charts@1e9d96d))
Summary
close #135
This PR addresses the issues with the previous
scaleToExtent
logic assuming a domain with only positive values. Now, whenscaleToExtent
is false, we return a domain dependent on whether the domain has positive or negative values. Previously, a chart with only negative values would only see the values rendering ifscaleToExtent
was true; now even whenscaleToExtent
is false, the negative values will render. For mixed negative & positive values, we return the original domain.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.