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

Fix axis automargin push offset #3510

Merged
merged 4 commits into from Feb 5, 2019

Conversation

Projects
None yet
3 participants
@etpinard
Copy link
Member

commented Feb 4, 2019

fixes #2985

I attempted to fix this bug by addressing #1988 and by association #2434, but that turned out to be too much of an endeavour. The DDK folks seems to really not like #2985, so here's a quick(er) fix.

In brief, we were previously assuming that the ticks + tick labels started right on the edge of the axis line when computing the auto-margin push value. This is incorrect, there's a slight pad between tick labels and ticks that must be taken into account to get the correct push.

See updated bug report codepens:

cc @plotly/plotly_js

etpinard added some commits Feb 4, 2019

fix axis automargin push offset value
- previously, we consider the ticks + tick labels to start right
  on the edge of the axis line. This is incorrect, there's a slight
  pad between tick labels and ticks that must be taken into account
  to get the correct automargin push
@antoinerg

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2019

Very nice PR @etpinard ! Thanks for fixing this issue 💃

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

Thanks for the review @antoinerg !

It might be nice to have some of the DDK folks try out

https://25536-45646037-gh.circle-artifacts.com/0/plotly.min.js

before merging this cc @cldougl @wbrgss

@wbrgss

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

This is incorrect, there's a slight pad between tick labels and ticks that must be taken into account to get the correct push.

Yeah, that's what it looked like to me as well. I've tested the above plotly.min.js with DDK, and can confirm this PR appears to fix #2985. This will be a great help; thank you @etpinard!

@etpinard

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

Thanks @wbrgss for checking! Merging 🚀

I'll release 1.44.3 tomorrow with this patch included.

@etpinard etpinard merged commit e3af586 into master Feb 5, 2019

8 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
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@etpinard etpinard deleted the fix-automargin-push-offset branch Feb 5, 2019

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.