The Wayback Machine - https://web.archive.org/web/20220519204108/https://github.com/plotly/plotly.js/pull/5208
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

Fix positioning monthly tickformat when initial auto dtick is weekly #5208

Merged
merged 9 commits into from Oct 15, 2020

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 10, 2020


if(prevDtick !== ax.dtick) {
// move tick0 back
ax.tick0 = axes.tickIncrement(ax.tick0, prevDtick, !axrev, ax.calendar);
Copy link
Contributor

@alexcjohnson alexcjohnson Oct 10, 2020

Choose a reason for hiding this comment

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

This seems problematic - if you set a specific tick0 it won't be honored? In any event I don't think we can be pushing a different value back into ax at this point, but if tickFirst needs a different effective start point perhaps we could make it an option to tickFirst?

Copy link
Contributor Author

@archmoj archmoj Oct 14, 2020

Choose a reason for hiding this comment

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

Good call. Addressed in a4cb02b.

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Oct 14, 2020

I don't really understand what's going on here, but strange things happen when I start interacting with this graph - a tiny drag and the ticks jump back to the incorrect "before" position. I think we need to resolve this at an earlier stage in the process.

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Oct 14, 2020

I don't really understand what's going on here, but strange things happen when I start interacting with this graph - a tiny drag and the ticks jump back to the incorrect "before" position. I think we need to resolve this at an earlier stage in the process.

Good catch. In fact it was previous commit a4cb02b that contributed in this bug.
For now I reverted part of it in ecb5067.
Now investigating if we could possibly resolve it earlier in the process.
Thanks.

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Oct 14, 2020

@alexcjohnson please note that the tick0 tweaking part was removed in 5d61cef.

Then commit 77a51d3 was enough to fixe the bug.

And c42358c as well as b78a80b basically moved most of the period logic into functions i.e. to help minimize the calcTicks function.

@alexcjohnson
Copy link
Contributor

@alexcjohnson alexcjohnson commented Oct 15, 2020

Much cleaner! But there's still one issue here, because the auto dtick prior to considering tickformat is weeks, the auto tick0 is '2000-01-02', hence the tick you see is on Jan 2. But then when you zoom out so the auto dtick is months, tick0 and the ticks you see jump back to the first of the month.

Seems to me like either adjustPeriodDelta needs to also adjust tick0, or perhaps even cleaner, the whole adjustPeriodDelta logic could be pushed into axes.autoTicks?

@archmoj
Copy link
Contributor Author

@archmoj archmoj commented Oct 15, 2020

Much cleaner! But there's still one issue here, because the auto dtick prior to considering tickformat is weeks, the auto tick0 is '2000-01-02', hence the tick you see is on Jan 2. But then when you zoom out so the auto dtick is months, tick0 and the ticks you see jump back to the first of the month.

Seems to me like either adjustPeriodDelta needs to also adjust tick0, or perhaps even cleaner, the whole adjustPeriodDelta logic could be pushed into axes.autoTicks?

Good eye.

@archmoj archmoj requested a review from alexcjohnson Oct 15, 2020
Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

💃 🎯

@archmoj archmoj merged commit d173d7a into master Oct 15, 2020
9 checks passed
@archmoj archmoj deleted the more-period-labels branch Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants