Fix charting lodash shenanigans #64
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
December 13, 2018 12:19
That is what it is.
It breaks lodash. We were not quite able to figure out what's going on, but apparently this plugin, while it does improve our bundle size slightly, also makes a total mess of (some?) lodash methods, which led to very strange behavior when RHKC was used in DHC (and probably elsewhere). Remove the plugin, and rely on the built-in Array.flatMap since that's simpler (we should only rely on lodash where necessary). Also some minor refactoring in related code, and moves lodash from devDependencies where we mistakenly had it to a proper dependency.
idan
approved these changes
Dec 13, 2018
| @@ -74,7 +73,8 @@ export default class HKLineChartData extends React.PureComponent< | |||
| ] | |||
|
|
|||
| // Domain of y coordinates (value) | |||
| const valueExtent = d3array.extent(values) | |||
| const values = data.flatMap(d => d[1]) | |||
| const [minVal, maxVal] = d3array.extent(values) | |||
There was a problem hiding this comment.
destructuring borat_very_nice.gif
Array.prototype.flatMap is not defined in node, where our tests run, so relying on the native flatMap works except in our tests. JavaScript is a good language.
msakrejda
commented
Dec 13, 2018
| @@ -95,7 +95,7 @@ export default class HKLineChartData extends React.PureComponent< | |||
| const area = d3shape | |||
| .area() | |||
| .x(d => xScale(d.x)) | |||
| .y0(yScale(valueExtent[0] < 0 ? yScale(valueExtent) : 0)) | |||
| .y0(yScale(Math.min(minVal, 0))) | |||
There was a problem hiding this comment.
Pretty sure this was bogus--if the min was less than zero, this would call yScale twice 😮
msakrejda
commented
Dec 13, 2018
| children={`${timeStamp}`} | ||
| /> | ||
| <HKTooltip xPos={hoverPos} yPos={height / 3}> | ||
| {timeStamp} |
There was a problem hiding this comment.
Let's pass children as children rather than a prop
idan
approved these changes
Dec 13, 2018
| @@ -95,7 +95,7 @@ export default class HKLineChartData extends React.PureComponent< | |||
| const area = d3shape | |||
| .area() | |||
| .x(d => xScale(d.x)) | |||
| .y0(yScale(valueExtent[0] < 0 ? yScale(valueExtent) : 0)) | |||
| .y0(yScale(Math.min(minVal, 0))) | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes https://trello.com/c/C48rWPXz/183-charts-calculate-boundaries-incorrectly-in-staging