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(axis): enable customization of tickLineProps #1211

Merged
merged 11 commits into from
Jul 8, 2021
Merged

new(axis): enable customization of tickLineProps #1211

merged 11 commits into from
Jul 8, 2021

Conversation

LoiKos
Copy link
Contributor

@LoiKos LoiKos commented May 13, 2021

💥 Breaking Changes

  • strokeWidth is used as default tick stroke width (can be override)

📝 Documentation

add support to configure stroke width:

  • tickStrokeWidth (defaultValue: undefined, optional) define stroke width

  • strokeWidth(default, defaultValue: 1) tick stroke width is equal to parent strokeWidth

@LoiKos
Copy link
Contributor Author

LoiKos commented May 13, 2021

I could improve this by using tickLinePropsas suggested but where can i find LineProps type ?

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 PR @LoiKos 🙌 I think this is looking good and thanks for the added tests! 🙏

I left a comment about an approach for tickLineProps if you want to give that a shot. Let me know if you have any other questions about it.

@@ -34,7 +36,7 @@ export default function Ticks<Scale extends AxisScale>({
className={cx('visx-axis-tick', tickClassName)}
transform={tickTransform}
>
{!hideTicks && <Line from={from} to={to} stroke={tickStroke} strokeLinecap="square" />}
{!hideTicks && <Line from={from} to={to} stroke={tickStroke} strokeWidth={tickStrokeWidth ?? strokeWidth} strokeLinecap="square" />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that this could be slightly more generic if we add tickLineProps, the props for <Line /> are currently a combination of specific LineProps and all SVGLineElement props as defined here.

The union currently isn't exported, but you should be able to access them with React.ComponentProps, something like

type LineProps = Omit<React.ComponentProps<typeof Line>, 'to' | 'from'>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will give a shot on adding LineProps

@LoiKos
Copy link
Contributor Author

LoiKos commented May 14, 2021

I have pushed a new version with tickLineProps. I wait for your feedback on this :)

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.

This looks great to me! 🥳 I had one small suggested fix for a stale comment but otherwise we can land this 🚢

packages/visx-axis/src/types.ts Outdated Show resolved Hide resolved
@williaster williaster changed the title Configurable stroke width new(axis): enable customization of tickLineProps May 14, 2021
Co-authored-by: Chris Williams <williaster@users.noreply.github.com>
@@ -11,6 +12,8 @@ export type AxisScale<Output extends AxisScaleOutput = AxisScaleOutput> =
// eslint-disable-next-line @typescript-eslint/no-explicit-any
D3Scale<Output, any, any>;

type LineProps = Omit<React.ComponentProps<typeof Line>, 'to' | 'from'>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

oof, looks like this somehow results in a very complex type such that TS throws

image

I think if we simplify this to just type LineProps = Omit<React.SVGProps<SVGLineElement>, 'to' | 'from'>; (TS seems to throw if we don't omit to/from because the types are different for the native SVGLineElement) and remove the import Line from '@visx/shape/lib/shapes/Line'; line above we should be good. Sorry for the back and forth on this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed this :)

@williaster
Copy link
Collaborator

Sorry looks like our happo/visual diffing CI is returning some errors at the moment and failing the build. I reached out to our contact to help debug.

@williaster
Copy link
Collaborator

williaster commented May 17, 2021

hey @LoiKos sorry for the CI troubles. we think this will be fixed if you force push the branch, so maybe you could rebase on the latest master?

If you're over it let me know and I can take over the PR to figure out any lingering issues 😅

@LoiKos
Copy link
Contributor Author

LoiKos commented May 17, 2021

Ok no problem i'll do that tomorrow

@LoiKos
Copy link
Contributor Author

LoiKos commented May 18, 2021

i have merge master into my branch is it possible to restart build ?

@williaster
Copy link
Collaborator

hey @LoiKos it should restart as soon as you push your branch, I'll keep an eye on it once I see the new commit!

@LoiKos
Copy link
Contributor Author

LoiKos commented Jul 8, 2021

Hey @williaster , i updated the branch to follow last change on visx/master. Could you take a look if you have a moment 😄 ?

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 @LoiKos ! This lgtm 🎉

@williaster
Copy link
Collaborator

We're having auth issues with happo but the other CI checks look good 👍 thanks again for the contribution! 🙏

@williaster williaster merged commit 330ebfc into airbnb:master Jul 8, 2021
@github-actions
Copy link

github-actions bot commented Jul 8, 2021

🎉 This PR is included in version v1.17.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.

2 participants