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

Property ticklabelindex for custom tick label display #7036

Merged
merged 35 commits into from
Jul 10, 2024

Conversation

my-tien
Copy link
Contributor

@my-tien my-tien commented Jun 25, 2024

This PR introduces the property drawminorticklabel ticklabelindex for drawing the label for each minor tick n positions away from a major tick

  • has no effect for category/multicategory/log axes
  • has no effect if tickformat frequency incompatible with minor tick frequency

One use case for this feature are period axes where we want to configure which period is labeled.

Current behavior

ticklabelindex_example_current

With "ticklabelindex": -1

ticklabelindex_example_new

Disclaimer I am required to add that…

the software is provided "as is", without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. in no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.

my-tien and others added 5 commits June 25, 2024 11:04
@archmoj archmoj added feature something new community community contribution status: reviewable labels Jun 25, 2024
@alexcjohnson
Copy link
Collaborator

I thought based on the attribute name that this would be about labeling ALL the minor ticks, but it’s just about shifting the label away from the major tick. Is there a use case other than period ticks, where the label is already shifted but now you want to shift it the other way? If that’s the only purpose I’d suggest making this specific to period ticks, something like labelminorperiod=“first”|”last”?

@stephprobst
Copy link

@alexcjohnson: One use case for more options than last and first would be to display the label for a specific quarter within a year. Imagine a quarterly report, for which the authors want to put emphasis on the last data point.

  • Major tick interval: 1 year
  • Minor tick interval: 1 quarter
  • Last data point (to emphasize): Q2 2024

In this example it would be desirable to display the "Q2 24" label, but to have the major ticks follow the Gregorian calendar (ticks on 1st of January).

image

@alexcjohnson
Copy link
Collaborator

I see, that’s reasonable. And in this case moving all the tick labels to Q2 is the desired behavior, not just a side effect? Ie would you ideally prefer Q1 to be labeled on all the earlier years (and possibly also 2024) and just label Q2 on 2024, or is what you’ve drawn with every Q2 labeled in fact the goal?

@stephprobst
Copy link

For us this would be the desired behaviour. The rationale is, that you typically want to compare the Q2 value of this year with the Q2 values of previous years.

I don't see a use case for drawing different minor tick labels for different years. I also think something like this would make the interface relatively complex. Therefore I would propose to stay with the level of control My-Tien proposed. What do you think?

@alexcjohnson
Copy link
Collaborator

Ok great, just wanted to confirm that this is truly the intent. So yes, let’s keep the behavior as implemented.

I’d still like to discuss the name, as to me drawminorticklabel implies adding extra labels rather than moving the already expected labels. It’s also a bit long: @etpinard had what I think is a great rule that no attribute name should concatenate more than three words and this is four.

What about minor.shiftlabels? ie put it inside the minor container since it only applies when there are minor ticks, but we don’t label minor ticks so it’s clear the shift applies to major tick labels.

@stephprobst
Copy link

Great! I don't have a strong opinion on the name, but minor.shiftlabels may be confused with the new property, that is currently being developed in this PR.

How about minor.labeltodisplay?

@my-tien
Copy link
Contributor Author

my-tien commented Jun 27, 2024

Thanks for the feedback!

Yes, finding a name is one of the more difficult problems of this PR and any help is welcome!

If we stick to period axes only, how about minor.periodtolabel or minor.labeledperiod?

The only reason why I came up with the original rather generic name is that the feature worked out of the box for linear axes as well and in case someone has a use case for that in the future, the name would still be fitting (But I currently can't think of any use case).

…g drawminorticklabel"

This reverts commit 11fd3f1.

I thought label overlap with minor ticks was something I introduced with drawminorticklabel, but this was also possible before and is sometimes desired (check e.g. baseline image 24.png)
src/plots/cartesian/axes.js Outdated Show resolved Hide resolved
@archmoj archmoj added this to the v2.34.0 milestone Jul 4, 2024
@LiamConnors
Copy link
Contributor

What would be an example of this incompatibility? "has no effect if tickformat frequency incompatible with minor tick frequency"?

src/plots/cartesian/axes.js Outdated Show resolved Hide resolved
Check for period compatibility did not take reversed axis into account.
Also added reversed axis to mock.
src/plots/cartesian/axes.js Outdated Show resolved Hide resolved
draftlogs/7036_add.md Outdated Show resolved Hide resolved
src/plots/cartesian/axes.js Outdated Show resolved Hide resolved
@@ -1166,38 +1223,44 @@ axes.calcTicks = function calcTicks(ax, opts) {

tickVals = tickVals.concat(minorTickVals);

var t, p;
function setTickLabel(ax, tickVal) {
var t = axes.tickText(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename t variable inside this function as there is another t declared in the upper scope.

src/plots/cartesian/axes.js Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Jul 9, 2024

@my-tien Just a few comments to address and we should be good to merge this PR. 🌟

Copy link
Contributor

@archmoj archmoj left a comment

Choose a reason for hiding this comment

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

Excellent PR.
💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants