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

no implicit round #302

Merged
merged 1 commit into from
Apr 8, 2021
Merged

no implicit round #302

merged 1 commit into from
Apr 8, 2021

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Apr 8, 2021

This turns off implicit rounding that was happening in axes, tick, and rule. If you want rounding, you can turn it on by setting round: true on the scale, but it feels a bit odd to say round: false but still get rounding.

@mbostock mbostock requested a review from Fil April 8, 2021 01:02
@Fil
Copy link
Contributor

Fil commented Apr 8, 2021

It works to solve the collapsing ordinal scale issue #72; for a medium number of bars, it makes the inset a bit fuzzy (which is why we're rounding in the first place). Have you considered d3/d3-scale#243 instead?

@mbostock
Copy link
Member Author

mbostock commented Apr 8, 2021

This doesn’t address #72 — in this PR ordinal scales still default to round: true. The only thing this changes is that when round = false, this removes implicit rounding that was happening inside the rule and tick marks, and within the axis. The axis rounding only applied to quantitative scales (which have scale.interpolate set to d3.interpolateRound).

So, I expect we want d3/d3-scale#243 in addition to this PR. And we might also want to default round to false for ordinal scales, even if that means fuzzy bars, or perhaps we want a new round = "auto" which defaults to true only if the cardinality of the ordinal domain is low enough that the behavior is reasonable.

@Fil
Copy link
Contributor

Fil commented Apr 8, 2021

OOps right, I tested on a binned bar chart 🤭

@mbostock mbostock merged commit 72fae8d into main Apr 8, 2021
@mbostock mbostock deleted the mbostock/noround branch April 8, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants