Repository navigation
fix: Center period labels over their own period when using ticklabelindex - #7934
Conversation
Previously, the ticklabelindex property itself was used in boolean tests which can be confusing.
camdecoster
left a comment
camdecoster
left a comment
There was a problem hiding this comment.
Most of this works, but there are a few changes that need to be made. I'm happy to talk through how to do some of this.
| // for period label positioning when using `ticklabelindex`: | ||
| // for each tick in `allTicklabelVals` holds the neighboring period end tick | ||
| var periodEndTicks; |
There was a problem hiding this comment.
Instead of saving an array of end ticks for each tick, what if we save the end tick to each tick? Then you could check for an end tick on each tick and use a different code path. You'd also be able to get rid of the new arg in positionPeriodTicks.
There was a problem hiding this comment.
Let me know if I didn't implement it as you intended.
| // This minor tick will be labeled instead of the major tick. | ||
| if (isPeriod) periodEndTicks = []; // for each minor tick at the start of a labeled period this will hold the neighboring period end tick. | ||
|
|
||
| const labelTickValsAscending = minorTickVals |
There was a problem hiding this comment.
What should happen if there's a major tick that doesn't coincide with a minor tick? Should that also be included?
There was a problem hiding this comment.
No, when ticklabelindex is used, only minor ticks should labeled. This step is just to handle the special case when a minor tick coincides with a major tick in which case the major tick should be labeled instead (because the minor could be removed later).
There was a problem hiding this comment.
Even if the major ticks aren't displayed, they're still used when determining which tick to display. From the description for ticklabelindex:
draw the label for the minor tick that is n positions away from the major tick
Without that major tick, the count will be off.
There was a problem hiding this comment.
Thanks, now all major ticks and all non-overlapping minor ticks are in allTicklabelVals.
camdecoster
changed the title
…odTicks - use ax._definedDelta in both cases, it would have been the same
camdecoster
left a comment
camdecoster
left a comment
There was a problem hiding this comment.
There are still a couple of things to address. I added some more context to my earlier comments.
ALso, could you please do a pass to make sure you're not using var in any of the new code?
| // This minor tick will be labeled instead of the major tick. | ||
| if (isPeriod) periodEndTicks = []; // for each minor tick at the start of a labeled period this will hold the neighboring period end tick. | ||
|
|
||
| const labelTickValsAscending = minorTickVals |
There was a problem hiding this comment.
Even if the major ticks aren't displayed, they're still used when determining which tick to display. From the description for ticklabelindex:
draw the label for the minor tick that is n positions away from the major tick
Without that major tick, the count will be off.
If no tickformat is specified, ax._actualDelta wasn't set and so periodX was also not set inside positionPeriodTicks.
…for < 1 day periods.
2 participants
Fixes #7876 by rewriting the way period label positioning works when using ticklabelindex.
Previously, the next labeled tick was used as the period end for the previous tick. This was problematic when using ticklabelindex, because sometimes the next labeled period is not necessarily adjacent to the last labeled period.
Now positionPeriodTicks can accept an array of periodEndTicks instead.