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

Add `tickson: 'boundaries'` for category cartesian axes #3275

Merged
merged 10 commits into from Nov 29, 2018

Conversation

Projects
None yet
2 participants
@etpinard
Copy link
Member

etpinard commented Nov 20, 2018

Closes #2601 - adding tickson to date axes and special spike support should get issues of their own.

cc @plotly/plotly_js

etpinard added some commits Nov 20, 2018

coerce tickson,
... only on `type: 'category'` cartesian axes with
    visible ticks and/or grid lines.
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Nov 20, 2018

Do we want a longer default ticklen when you turn on tickson: 'boundaries'? And we definitely need to not push the labels away from the axis (ie to be beyond the ticks).

There are probably also some unpleasant effects on collision avoidance. Imagine the case:
'A very long title', 'short', 'Another very long title'
They could all avoid touching each other yet overlap the boundaries which would still be unacceptable, at least if we're showing ticks, though it might still be better to rotate in this case even with just grid lines. And perhaps we want to only auto-rotate to 90 degrees, omitting the step at 30 degrees?

@etpinard

This comment has been minimized.

Copy link
Member Author

etpinard commented Nov 22, 2018

Thanks very much @alexcjohnson for the thorough review. I overlooked many subtle details. 🤕

And we definitely need to not push the labels away from the axis (ie to be beyond the ticks).

Oh right. Done in -> 78ff4d5

There are probably also some unpleasant effects on collision avoidance. Imagine the case:
'A very long title', 'short', 'Another very long title'
They could all avoid touching each other yet overlap the boundaries which would still be unacceptable, at least if we're showing ticks, though it might still be better to rotate in this case even with just grid lines. And perhaps we want to only auto-rotate to 90 degrees, omitting the step at 30 degrees?

I went with auto-rotating straight to 90 degrees with or w/o ticks in -> 09ac230

Do we want a longer default ticklen when you turn on tickson: 'boundaries'?

I'm not opposed. How much longer are you thinking?

Show resolved Hide resolved src/plots/cartesian/axes.js Outdated
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Nov 26, 2018

Do we want a longer default ticklen when you turn on tickson: 'boundaries'?

I'm not opposed. How much longer are you thinking?

Hmm, looking at it in more detail, I think perhaps the current default is fine. Definitely once we get to multi-level axes, ticks for the outer levels will need to by default extend beyond the inner level's labels, but we can leave it as is for now.

}
};
for(i = 0; i < vals.length; i++) _push(vals[i], 0);
_push(vals[i - 1], 1);

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Nov 26, 2018

Contributor

nice solution, keeping xbnd with the rest of the tick calculations.

"x": ["A very long title", "short", "Another very long title"],
"y": [1, 4, 2],
"xaxis": "x4",
"yaxis": "y4"

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Nov 26, 2018

Contributor

Nice, just squeaks in unrotated if we draw this without tickson: 'boundaries' 🎉
screen shot 2018-11-26 at 1 20 37 pm

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

alexcjohnson commented Nov 28, 2018

OK, looks great! 💃

@etpinard etpinard merged commit e930009 into master Nov 29, 2018

7 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details

@etpinard etpinard deleted the tickson-boundary branch Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.