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

Implement ticklabelstep on 2D axes & colorbars #6088

Merged
merged 26 commits into from Feb 4, 2022
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Jan 21, 2022

It sets the step between tick labels as an integer number.
This feature can help hide labels between every n-th ticks.
@plotly/plotly_js


var jump = ax.ticklabeljump;
var vals = jump ?
opts.vals.filter(function(_, i) { return i % (jump + 1) === 0; }) :
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding this correctly, this way we're always going to get the first label that appears in the range, then every nth after that. But what we really want is to always get the label corresponding to tick0 if that's within the range, otherwise start n ticks away from tick0. An important consequence of this is that as you pan around on a graph the same label will remain displayed or not displayed as long as it's in range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certain filtering logic is now moved from drawLabels to calcTicks by 29ab444.
Next I'd try to improve on that to keep the same labels around...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now addressed by 3b259ed.

@archmoj archmoj added this to the v2.9.0 milestone Jan 25, 2022
editType: 'ticks',
description: [
'Sets the step between ticklabels.',
'Could be used to hide labels between every n-th ticks.'
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that dticklabel isn't a good name as it implies it should have the same units as dtick instead of being an integer multiplying dtick. I don't love jump though. Some thoughts:

  • ticksperlabel (feels to me like the most precise name, and it's fairly concise, but it sort of makes it look like the ticks are modified rather than the labels)
  • skipticklabels (but then it should mean "how many do we skip," ie 1 should mean "show every 2nd" and 0 should be the default behavior - add one to get the current meaning)
  • any other suggestions?

If we keep the current meaning, the default and min should be 1 instead of 0. Either way the description could be made more explicit, something like:

Sets the spacing between tick labels as compared to the spacing between ticks. A value of 1 (default) means each tick gets a label. A larger value n means only every nth tick is labeled. tick0 determines which labels are shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I uses skipticklabels in 2676d24.
But how about ticklabelpace?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, let's stick with skipticklabels - I still don't love it but it's the best we have so far.

@archmoj archmoj changed the title Implement ticklabeljump on 2D axes & colorbars Implement skipticklabels on 2D axes & colorbars Feb 1, 2022
@alexcjohnson
Copy link
Contributor

Behavior looks correct to me for linear and category axes 🎉 but needs some work on date axes 🚧

It doesn't look like we're honoring tick0 correctly in all cases. For example here's the date_axes mock with all axes set to skipticklabels=1, notice that for the bottom axis (xaxis) we lose the label at year 2000 (which is tick0), and for xaxis5 we lose the midnight label, which is an integer number of 6-hour intervals from tick0 so it should appear.

Also when I drag these axes around the displayed ticks are NOT stable, in quite a few cases we flip to displaying the alternate set of labels.

Finally, if the removed labels are the only ones that showed the second line of the label, the second line should be shown for the remaining labels using the same logic as we would normally use if the labeled ticks were the only ticks on the graph. You can see this on xaxis5 for example - both of these labels should keep their second lines. I think this means basically we need to run the routine that drops the second line after we remove the labels we aren't going to display, and only considering the labels that remain.

Screen Shot 2022-02-02 at 11 04 51 AM

@archmoj
Copy link
Contributor Author

archmoj commented Feb 3, 2022

Behavior looks correct to me for linear and category axes tada but needs some work on date axes construction

It doesn't look like we're honoring tick0 correctly in all cases. For example here's the date_axes mock with all axes set to skipticklabels=1, notice that for the bottom axis (xaxis) we lose the label at year 2000 (which is tick0), and for xaxis5 we lose the midnight label, which is an integer number of 6-hour intervals from tick0 so it should appear.

Also when I drag these axes around the displayed ticks are NOT stable, in quite a few cases we flip to displaying the alternate set of labels.

Finally, if the removed labels are the only ones that showed the second line of the label, the second line should be shown for the remaining labels using the same logic as we would normally use if the labeled ticks were the only ticks on the graph. You can see this on xaxis5 for example - both of these labels should keep their second lines. I think this means basically we need to run the routine that drops the second line after we remove the labels we aren't going to display, and only considering the labels that remain.

Screen Shot 2022-02-02 at 11 04 51 AM

Good calls!
Please see 3734680 and 3d46d98.
Tests are also added by a72616a.

@archmoj archmoj changed the title Implement skipticklabels on 2D axes & colorbars Implement ticklabelstep on 2D axes & colorbars Feb 3, 2022
@archmoj
Copy link
Contributor Author

archmoj commented Feb 3, 2022

@alexcjohnson this PR is ready for another round of review 🙏 SVP.

Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Fantastic!

@archmoj archmoj merged commit 74a8f9c into master Feb 4, 2022
@archmoj archmoj deleted the ticklabeljump branch February 4, 2022 17:02
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.

None yet

2 participants