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

consistent intervals with interval.every #66

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Aug 18, 2023

Fixes #63. Fixes #64. We need to finish the documentation, and decide whether it is acceptable that d3.timeHour.every(…) is no longer aligned at midnight.

TODO

@mbostock mbostock requested a review from Fil August 18, 2023 18:40
@Fil
Copy link
Member

Fil commented Aug 21, 2023

Seems like a net positive — the current intervals are v. surprising when not strict divisors of 24 —, but indeed a breaking change.

Playing with this in this notebook I can see how it could be surprising for users who expect 2, 3, 4, 6 and 12-hours periods to align with midnight; I've tried to add an “align” option (we could add this as a hint in the documentation or release notes). Putting in a DST change to make things more difficult :)

@mbostock
Copy link
Member Author

I don’t think it’s worth the trouble to consider this a major version bump across D3. The current behavior is broken in many cases. In the case of UTC ticks, the behavior is essentially unchanged. And for local time ticks, the only difference you can see is the two-days ticks are now uniformly spaced rather than resetting on odd-numbered months.

Before:
Screenshot 2023-08-21 at 9 43 22 AM

After:
Screenshot 2023-08-21 at 9 43 03 AM

The downside is that there’s no guarantee now that you will see the “February” because Feb 1 can get skipped in favor of Feb 2. But the default time ticks in D3 are already so much more confusing than Plot’s approach. I think this is a net improvement.

Notebook: https://observablehq.com/d/8c469f7949f95a23

@mbostock
Copy link
Member Author

mbostock commented Aug 21, 2023

Also, we already changed the visible behavior of utcTicks by adopting unixDay instead of utcDay in #58, which is why this PR doesn’t change the visible behavior of utcTicks; if we did the same thing for local time as described in #62 then this PR wouldn’t change the visible behavior of timeTicks, either.

Of course, it is a difference that we are talking about not just changing the behavior of utcTicks/timeTicks but the intervals themselves, but it’s also confusing to have two versions of every interval, and for utcTicks/timeTicks to use the newer (and less well-known) intervals to avoid the surprising behavior, while the existing (and most well-known) intervals still behave surprisingly.

@Fil
Copy link
Member

Fil commented Aug 21, 2023

lgtm!

@mbostock mbostock marked this pull request as ready for review August 22, 2023 00:15
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.

interval.every can return intervals of inconsistent length
2 participants