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

Specifying tick label props should not wipe all defaults #1657

Closed
Robin-Hoodie opened this issue Feb 15, 2023 · 4 comments · Fixed by #1662
Closed

Specifying tick label props should not wipe all defaults #1657

Robin-Hoodie opened this issue Feb 15, 2023 · 4 comments · Fixed by #1662

Comments

@Robin-Hoodie
Copy link
Contributor

Now, when setting something like

tickLabelProps={() => ({
   stroke: "red"
})};

will remove all tick label props that had been set by default, which is probably not what most users want.
To make matters worse, the default props are not exported from @visx/scale so users end up having to look through the source code and copy-paste the default props in their own projects.

Lastly, it would help if you'd be allowed to specify tickLabelProps as a plain object or a function, instead of just as a function.

Happy to make a PR for this if this is something people can agree on

@williaster
Copy link
Collaborator

Happy to review a PR for all this, thanks for suggesting it!

Note that this will likely require updates in @visx/axis, @visx/xychart, and @visx/react-spring (where we export animated versions of Axis*). I think the majority of the work would be in @visx/axis and the others will just need minimal type propagation. I don't think this needs to be a breaking change.

@Robin-Hoodie
Copy link
Contributor Author

Robin-Hoodie commented Feb 16, 2023

@williaster I've forked the repo and did some work on this, though whenever I run yarn build from the root of the repo after making some changes, I get the below error. I've taken a look at @visx/point and tried to run yarn tsc --build just in that repo itself, and for some reason its typings are not being emitted. This does not happen when I run yarn build from a freshly cloned repo, though if I apply some changes there, the same thing happens again.

I this something you've seen before? I want to avoid potentially spending hours looking for a fix that is seemingly not related to my changes

image

@williaster
Copy link
Collaborator

hey @Robin-Hoodie thanks for getting this started. hmm, I haven't seen that specific error before, but recently I've seen some issues with stale tsconfig.tsbuildinfo files and have fixed it with a rm packages/**/tsconfig.tsbuildinfo command (we could run this as part of the script ..).

if that doesn't work, maybe you can just rely on the CI from your PR and not worry about it locally? (not ideal I know)

@Robin-Hoodie
Copy link
Contributor Author

@williaster Removing the tsconfig.tsbuildinfo files worked, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants