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

axis.tickSize accepts a function #84

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

IPWright83
Copy link

* axis.tickSize accepts a function to allow changing the tickSize dynamically on a per tick basis
@IPWright83
Copy link
Author

The aim behind this is to help support more customised axis such as, which I believe should now be possible without having to rely on CSS nth selectors.

image

src/axis.js Outdated Show resolved Hide resolved
src/axis.js Outdated Show resolved Hide resolved
test/axis-test.js Outdated Show resolved Hide resolved
test/axis-test.js Outdated Show resolved Hide resolved
IPWright83 and others added 4 commits April 6, 2022 20:48
Pass function rather than manually calling

Co-authored-by: Mike Bostock <[email protected]>
Missing semi colon!

Co-authored-by: Mike Bostock <[email protected]>
Safe to use arrow function in unit tests

Co-authored-by: Mike Bostock <[email protected]>
Clear tickSizeInner and tickSizeOuter when setting tickSizeFunction

Co-authored-by: Mike Bostock <[email protected]>
@IPWright83
Copy link
Author

Thanks for the comments @mbostock , committed all those suggestions

Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

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

Very useful! A few nits below (the nodes and this are passed as a consequence of a3e89d2).

test/snapshots.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Fil
Copy link
Member

Fil commented Apr 6, 2022

What do you think about positioning the tick labels relatively to the ticks? Currently if using the function, the ticks "crash" into the axis, and I'm not sure how to fix that.

Capture d’écran 2022-04-06 à 23 14 51

Should we not also allow tickPadding to be a function, and set it to be the result of the tickSize function + a constant padding (unless it's already been set)?

You can build on the demo notebook by forking it:
https://observablehq.com/@d3/d3-axis-ticksize-function-84

To update the build, just run yarn rollup -c in the repo and replace the d3-axis.js file in the file attachments.

It would be great to add various examples making use the new feature.

IPWright83 and others added 2 commits April 7, 2022 07:36
Make the snapshot readable

Co-authored-by: Philippe Rivière <[email protected]>
Updated DOCS as additional parameters now passed

Co-authored-by: Philippe Rivière <[email protected]>
@IPWright83
Copy link
Author

IPWright83 commented Apr 7, 2022

@Fil thanks, I think you'd mentioned on Slack how to setup an Observable but I hadn't had much time. Decided I could just whip this PR together last night off the cuff. Yeah I can certainly add the example that I'm trying to build.

I think you're right about tickPadding. I think we should default it to exactly what you've suggested. I'll be honest when I've been using it, I've been disabling/moving labels too - which might be something useful for the future.

@IPWright83
Copy link
Author

Sorry, taken me a little while to get back to this. I've not added tickPadding as a function, as I couldn't foresee when you'd need to do that (and it started requiring nested ternaries which I'm not a fan of). But I've updated tickPadding to take the tickSizeFunction into account @Fil.

I've forked your example and add my use case in there (just showing major tick labels) https://observablehq.com/@ipwright83/d3-axis-ticksize-function-84 and thought of a crazy use case where you might want to move "longer labels" to prevent overlap.

src/axis.js Outdated Show resolved Hide resolved
@IPWright83
Copy link
Author

Sorry to message you direct @mbostock, but would love to be able to use this if possible without maintaining a fork.

Is there anything you'd like to change to get this in? Thanks!

Copy link
Contributor

@curran curran left a comment

Choose a reason for hiding this comment

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

Looks like a nice change to me!

@denisemauldin
Copy link

@mbostock This looks like a great PR. Any way we can help get this accepted?

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

Successfully merging this pull request may close these issues.

6 participants