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 ticklabeloverflow to improve control over display of tick labels #5584

Merged
merged 15 commits into from
Apr 14, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Apr 8, 2021

Addressing #5404 by adding new attribute to cartesian axes (and few more places than use cartesian axis e.g. indicator and colorbar, obviously not gl3d).

The options currently implemented are allow, hide past div, hide past domain.

The default value when ticklabelposition is set to inside is 'hide past domain'.
Otherwise on category and multicategory axes the default is 'allow'.
In other cases the "new" default (to fix the bug) is 'hide past div'.

This PR also paved the way so that other options such as push to div, and push to domain could be added later on the road to address #3292.

@plotly/plotly_js

@@ -136,6 +136,7 @@ module.exports = overrideAll({
tickvals: axesAttrs.tickvals,
ticktext: axesAttrs.ticktext,
ticks: extendFlat({}, axesAttrs.ticks, {dflt: ''}),
ticklabeloverflow: extendFlat({}, axesAttrs.ticklabeloverflow, {dflt: 'allow'}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't want a dflt here, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye. Addressed in f833ce4.

else if(pushOverflow) {
if(adjust.indexOf('left') !== -1) t.attr('text-anchor', 'start');
if(adjust.indexOf('right') !== -1) t.attr('text-anchor', 'end');
// more TODO: https://github.com/plotly/plotly.js/issues/3292
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could leave another branch open with this WIP for "push to ..." but I'd rather not have it commented out in the main codebase. Anyway the eventual solution is going to need to be a bit more complex than this: we'll want to push these labels only as far as necessary, and then if doing so causes them to intersect the next labels we should still hide them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by c9eaff2.

@alexcjohnson
Copy link
Collaborator

The new test images look great. I don't think we want the indicator mock to change though - if indicator labels overflow the div I think we should still show them.

@archmoj
Copy link
Contributor Author

archmoj commented Apr 13, 2021

The new test images look great. I don't think we want the indicator mock to change though - if indicator labels overflow the div I think we should still show them.

Good call. Done in 0deec98.

Copy link
Collaborator

@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.

Good to go! 💃

@nicolaskruchten
Copy link
Contributor

⭐ please merge!

@archmoj archmoj merged commit 296dbfb into master Apr 14, 2021
@archmoj archmoj deleted the ticklabeloverflow branch April 14, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants