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

fix(axis): include defaultTextProps in Axis labelProps #1791

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

RyKilleen
Copy link
Contributor

@RyKilleen RyKilleen commented Jan 19, 2024

🐛 Bug Fix

  • Enables passing of partial labelProps per the docs

Closes #1790

This enables passing of partial labelProps per the docs.
Copy link
Collaborator

@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.

Thanks for the fix here/making it consistent with tickLabelProps! (that behavior was added relatively recently in #1662)

Technically this could be thought of as a breaking change as it would change existing behavior if consumers expect to provide all props. However for tickLabelProps we just made it a minor version bump so we can do that here. I agree this is a better dev x.

packages/visx-axis/src/axis/AxisRenderer.tsx Outdated Show resolved Hide resolved
Co-authored-by: Chris Williams <williaster@users.noreply.github.com>
@RyKilleen
Copy link
Contributor Author

Thanks @williaster! Suggested changes applied.

Copy link
Collaborator

@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.

sorry missed one spot that I think needs to include the superset of props. else lgtm! thanks for the followup 🙏

packages/visx-axis/src/axis/AxisRenderer.tsx Outdated Show resolved Hide resolved
Co-authored-by: Chris Williams <williaster@users.noreply.github.com>
Copy link
Collaborator

@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.

ah fun, sorry hopefully third time's the charm :)

packages/visx-axis/src/axis/AxisRenderer.tsx Outdated Show resolved Hide resolved
Co-authored-by: Chris Williams <williaster@users.noreply.github.com>
@williaster williaster changed the title fix: include defaultTextProps in Axis labelProps fix(axis): include defaultTextProps in Axis labelProps Jan 25, 2024
Copy link
Collaborator

@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.

thanks again @RyKilleen ! 🙏

@williaster williaster merged commit 843d321 into airbnb:master Jan 25, 2024
2 checks passed
Copy link

🎉 This PR is included in version v3.8.0 of the packages modified 🎉

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.

<Axis /> labelProps is documented as Partial, but overwrites values
2 participants