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

new(vx-react-spring): add package + AnimatedAxis #779

Merged
merged 11 commits into from
Aug 21, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Aug 13, 2020

🚀 Enhancements

This PR adds a new AnimatedAxis component to @vx/axis and the needed react-spring dependency. The surface area is pretty minimal because only ticks are animated so we can fully re-use Axis/AxisBottom/etc and override a new ticksComponent prop which defaults to the old behavior.

I'm a little conflicted because generally vx is not opinionated on animation libraries and we don't want to bloat bundle sizes. However, the incentive for this comes from the XYChart POC in #745 where we would like to support animation if users opt in for it. So, we could either
a) simply add the needed Axis hooks (ticksComponent) to enable this cleanly, and keep AnimatedAxis in XYChart. or
b) add it to @vx/axis to let users opt into using it. It shouldn't affect bundle sizes if consumers use tree shaking or deep imports

To make b) a bit cleaner we could consider

  • avoid exporting AnimatedAxis from the index and require a deep import
  • add react-spring as a peer-dep if we want consumers to be able to use an existing version they have

UPDATE based on feedback

@vx/axis

  • @vx/axis Axis is refactored to accept a ticksComponent which allows us to animate them
  • adds a third argument values to tickFormat(value, index, values) so that format logic can more easily leverage all ticks (because numTicks is approximate, lib consumers do not know how many tick values exist a priori)

@vx/react-spring

  • adds a new package @vx/react-spring that includes react-spring as a peerDep and can be a home for things that depend on react-spring
  • Adds an <AnimatedAxis /> and <AnimatedTicksRender /> in @vx/react-spring
  • updates the vx-demo/axis demo to use <AnimatedAxis />

Updated /axis demo

@hshoff @kristw @techniq

"prop-types": "^15.6.0"
},
"devDependencies": {
"@vx/scale": "0.0.198"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is now a dep so don't need to duplicate it

return animatedTicks.map(({ item, key, props }, index) => {
// @ts-ignore react-spring types don't handle fromX, etc.
const { fromX, toX, fromY, toY, opacity } = props;
const tickLabelProps = allTickLabelProps[index] ?? allTickLabelProps[0] ?? {};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't come up with a great solution for this. when tick values update, there may not be a tickLabelProps for that index.

ultimately the weirdness comes from the Axis component trying to find the max tick label font size to correctly offset the axis label (will comment on this). maybe we should deprecate that logic and clean this up?

index,
from: createPoint({ x: scaledValue, y: 0 }, horizontal),
to: createPoint({ x: scaledValue, y: tickLength * tickSign }, horizontal),
formattedValue: format(value, index, filteredTickValues),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

have been wanting to add this for a while (will update the PR description): we now pass all ticks as a third argument (common d3 pattern) so that users can determine if a tick is first/last, etc. with index only you couldn't ever determine this because you may not know the exact # ticks unless you specify tickValues

axisClassName,
labelOffset = 8,
tickLabelProps = (/** tickValue, index */) => ({
export const bottomTickLabelProps = (/** tickValue, index */) =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

exporting these so that they could be used outside the component. tick label props are the primary differences between axis configurations

let tickLabelFontSize = 10; // track the max tick label size to compute label offset

// compute the max tick label size to compute label offset
const allTickLabelProps = ticks.map(({ value, index }) => tickLabelProps(value, index));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the jank logic I alluded to above. The label and ticks were previously coupled with this, so I updated the logic to pass an array of tickLabelProps to ticksComponent.

Might be worth a breaking change to remove this?

numTicksColumns={numTickColumns}
/>
<AnimatedAxis
orientation={Orientation.bottom}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is all just indented and we use AnimatedAxis + specify orientation

@hshoff
Copy link
Member

hshoff commented Aug 19, 2020

I'd like to propose option c): create a new package @vx/react-spring that includes react-spring as a peerDep and can be a home for things that depend on react-spring and exports things like <AnimatedAxis />

@williaster
Copy link
Collaborator Author

🤯 I dig it! Noting that I think a lot of the XYChart elements might end up living there like AnimatedBars, AnimatedStackedBars, etc. (likely animated equivalents of many things in @vx/shape).

Organizing that over time might be tricky, but I think it'll be a good forcing function to keep things modular / might converge on good patterns over time.

@williaster williaster changed the title new(vx-axis): add AnimatedAxis new(vx-react-spring): add package + AnimatedAxis Aug 19, 2020
@williaster
Copy link
Collaborator Author

williaster commented Aug 19, 2020

doc page for the new package

image

Copy link
Member

@hshoff hshoff left a comment

Choose a reason for hiding this comment

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

Coverage decreased (-0.5%) to 44.264%

Let's add some tests, could just be simple exports/defined checks.

@williaster
Copy link
Collaborator Author

@hshoff 🎉

Coverage increased (+2.2%) to 46.97%

I have an AnimatedGrid PR stacked on this, will add some more legit ones there

"@vx/scale": "0.0.198",
"@vx/text": "0.0.198",
"classnames": "^2.2.5",
"prop-types": "^15.6.2"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need prop-types here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, nimbus has a parser that adds propTypes to components during the build based on the types.

Comment on lines +25 to +50
const fromLeave = ({ from, to, value }: ComputedTick<Scale>) => {
const scaledValue = scale(value) ?? 0;

return {
fromX: horizontal
? // for top/bottom scales, enter from left or right based on value
scaledValue < scaleLength / 2
? minPosition
: maxPosition
: // for left/right scales, don't animate x
from.x,
// same logic as above for the `to` Point
toX: horizontal ? (scaledValue < scaleLength / 2 ? minPosition : maxPosition) : to.x,
// for top/bottom scales, don't animate y
fromY: horizontal
? // for top/bottom scales, don't animate y
from.y
: // for left/right scales, animate from top or bottom based on value
scaledValue < scaleLength / 2
? minPosition
: maxPosition,
// same logic as above for the `to` Point
toY: horizontal ? to.y : scaledValue < scaleLength / 2 ? minPosition : maxPosition,
opacity: 0,
};
};
Copy link
Member

Choose a reason for hiding this comment

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

Would something like this work? would be great to avoid nested ternaries

const fromLeave = ({ from, to, value }: ComputedTick<Scale>) => {
  const scaledValue = scale(value) ?? 0;
  const lhs = scaledValue < scaleLength / 2;
  const opacity = 0;

  // vertical
  let fromX = from.x;
  let toX = to.x;
  let fromY = lhs ? minPosition : maxPosition;
  let toY = lhs ? minPosition : maxPosition;

  if (horizontal) {
    fromX = lhs ? minPosition : maxPosition;
    toX = lhs ? minPosition : maxPosition;
    fromY: from.y;
    toY: to.y;
  }

 return { opacity, fromX, toX, fromY, toY };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree this isn't great, but I cleaned it up / refactored a bunch in #787 here.

@williaster williaster merged commit ef2db11 into master Aug 21, 2020
@williaster williaster deleted the chris--animated-axis branch August 21, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants