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

Add axis.offset #76

Merged
merged 3 commits into from
Mar 10, 2021
Merged

Add axis.offset #76

merged 3 commits into from
Mar 10, 2021

Conversation

mbostock
Copy link
Member

What do you think about making the current 0.5px offset configurable, and defaulting it to zero on devices with a devicePixelRatio ≥ 2? The purpose of this offset is to provide pixel-snapping, but that’s only necessary when the devicePixelRatio = 1, and these days, that’s uncommon.

@mbostock mbostock requested a review from Fil February 21, 2021 18:55
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.

LGTM. Solid move.

@curran
Copy link
Contributor

curran commented Feb 22, 2021

Related to #75

@Fil
Copy link
Member

Fil commented Feb 23, 2021

I work daily on a density=1 desktop monitor :)

A density of 1.5 is quite common on Android phones, and it seems that it too would be better with offset = 0.

Here's the result of various settings:
https://observablehq.com/@d3/axis-offset-76

summary:

dpi offset result
1 0 bad
1 0.5 good
1.5 0 good
1.5 0.5 not so good
2 0 good
2 0.5 bad

1 / 0 😡
dpi1-offset0

1 / .5 😎
dpi1-offset0 5

1.5 / 0 😎
dpi1 5-offset0

1.5 / 1.5 😡
dpi1 5-offset0

2 / 0 😎
dpi2-offset0

2 / .5 😡
dpi2-offset0 5

(note that I used "download png", not screen grab :))

all.zip

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.

just this change for 1.5

src/axis.js Outdated Show resolved Hide resolved
Co-authored-by: Philippe Rivière <[email protected]>
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.

3 participants