Repository navigation
Conversation
my-tien
marked this pull request as ready for review
August 10, 2026 08:03
robertclaus
requested review from
KoolADE85 and
camdecoster
and removed request for
KoolADE85 and
emilykl
September 15, 2026 20:11
camdecoster
changed the title
ticklabelindex
camdecoster
requested changes
Sep 24, 2026
camdecoster
left a comment
camdecoster
left a comment
Contributor
There was a problem hiding this comment.
This looks good overall, but I'll reserve my final review for after the other two PRs in the stack are completed. Could you update the tick dedupe step to include the hidden minor ticks?
by adding an additional major tick at the end of the axis range + all minor ticks between the last visible tick and that helper. Previously, there was already code that added one major tick and one minor tick before tick0. Now the behavior is symmetrical to the axis range end (1 major tick and all minor ticks between that and tick0). We need all minor ticks between the first/last visible tick and the major helper tick so that walking ticklabelindex steps from that helper tick lands on the correct minor tick.
This is just to be consistent with the way major ticks are set. In axes_test we test for empty string, instead of for undefined.
…of last period)
my-tien
force-pushed
the
7875-ticklabelindex-last-period
branch
from
October 25, 2026 08:57
655f207 to
e6c7a5b
Compare
Contributor
Author
|
@camdecoster I wanted to download the images created in the CI to commit them, however, there seems to be an issue causing the test-baselines step to be skipped? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The period label placement logic always requires two enclosing ticks. Because in the past (before ticklabelindex), period labels only appeared to the right of a tick, plotly added an invisible helper tick before the first visible major tick, so that the period before the first visible tick could be labeled.
With a negative ticklabelindex it's also possible to label the period left of a tick. For that case, in order to label the period after the last visible tick we need a corresponding helper tick after that.
In addition, we need all minor ticks between the first/last visible tick and the major helper tick so that walking ticklabelindex steps from that helper tick lands on the correct minor tick.
Fixes #7875
Stacks on top of #7934, #7935.