-
Notifications
You must be signed in to change notification settings - Fork 729
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
new(xychart): add AreaStack #1019
Conversation
packages/visx-xychart/src/components/series/private/BaseBarStack.tsx
Outdated
Show resolved
Hide resolved
packages/visx-xychart/src/components/series/private/BaseAreaStack.tsx
Outdated
Show resolved
Hide resolved
|
||
const barSeriesChildren = useMemo( |
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.
all this logic is the same for AreaStack
and is moved to useStackedData
}, [combinedData, dataKeys, order, offset]); | ||
|
||
// update the domain to account for the (directional) stacked value | ||
const comprehensiveDomain = useMemo( |
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.
all of this is unchanged except comprehensiveDomain
is now computed post-stacking because different stack offset
s yield different data ranges.
Pull Request Test Coverage Report for Build 513437350Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
) as unknown) as DataContextType<XScale, YScale, StackDatum>; | ||
|
||
// find series children | ||
// @TODO: memoization doesn't work well if at all for this |
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.
what do you mean by doesn't work well? still slow?
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.
or is it because the equality check doesn't work well for children
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.
equality check doesn't work with children
, so I think basically everything in this hook is always re-run. I can probably optimize it with ref
s to the child data arrays (which would still break if users have inline data but hopefully not).
I plan to follow up on this in another PR with tests for it.
) as unknown) as DataContextType<XScale, YScale, StackDatum>; | ||
|
||
// find series children | ||
// @TODO: memoization doesn't work well if at all for this |
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.
or is it because the equality check doesn't work well for children
); | ||
|
||
// if scales and data are not available in the registry, bail | ||
if (dataKeys.some(key => dataRegistry.get(key) == null) || !xScale || !yScale || !colorScale) { |
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.
perhaps move the !xScale || !yScale || !colorScale
check before the dataKeys.some
because they are cheaper. Although dataKeys
is negligibly small.
* fix(xychart/BaseAxis): handle undefined scale * fix(xychart/useDimensions): fix partial initialDimensions * new(xychart/useScales): avoid NaN domain when no values are present * fix(xychart/DataProvider): no negative ranges or dimensions * fix(xychart/getScaleBandwidth): handle undefined scale case * test(xychart): add AreaStack, useStackedData tests
40c3855
to
eb30e36
Compare
🚀 Enhancements
This closes #994 and
AreaStack
series to support stacked areas + stream graphsBarStack
andAreaStack
logic in a newuseStackedData
hook🐛 Bug Fix
useStackedData
for different stackoffset
s (e.g.,wiggle
for streamgraphs; onlydiverging
andnone
worked previously)TODO (separate PRs to keep size down)
/xychart
demo new(demo/xychart): add AreaStack, add stackOffset variable for stacks #1020Preview images from the demo branch:
Basic

Tab-able tooltip

Stack offsets

@hshoff @kristw