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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix annotation autorange on category axes #1791

Merged
merged 5 commits into from
Jun 19, 2017
Merged

Conversation

etpinard
Copy link
Contributor

fixes #1768 by making the category axes setConvert methods a bit more of a mess. Note that annotation autorange uses ax.r2c.

Oh well, with this patch here, it seems like all the cases are covered.

Together with #1748, we now have working category annotations on first draw and on pan/zoom 馃帀 :

peek 2017-06-15 15-21

By the way @alexcjohnson I chose to add a new mock in f5c0382 instead of appending annotation-autorange.json to test shapes on category axes as well. I hope you don't mind.

- fixes annotations autorange
- does not appear to have any unwanted side-effects.
@alexcjohnson
Copy link
Collaborator

Nice and simple, and this actually seems like it's the logical way for ax.r2c to work. No problem about splitting off the mock - you're right to include both x and y, shapes and annotations (what about images?) - though at some point I feel like a bit of consolidation may be in order.

But did that mock fail before the patch? The coordinates that lead to the shape and annotations bumping out the autorange didn't come from category names. For annotations we can use a category that's already in the data and let the text box or arrow add extra pixel padding, for shapes it seems like we may need to use a category that's not in the data, so ax.categoryarray?

- same as what Axes.coercePosition does at the defaults step
  for other axis types
- post #1748, we need to handle this at the ax.r2c as the categories
  are only known after the calc step.
@etpinard
Copy link
Contributor Author

@alexcjohnson

Nice and simple, and this actually seems like it's the logical way for ax.r2c to work.

Great.

(what about images?)

Layout-image-induced autorange bumps aren't a thing ye (seet #1111) and the layout_image mock already has a category axis. But yeah, when #1111 will be completed, we should add an image or two to category-autorange.json.

But did that mock fail before the patch?

Oops, I messed that up when I added arrows to the mock. Fixed in -> e4aa7a3

for shapes it seems like we may need to use a category that's not in the data, so ax.categoryarray?

Good call here. That actually uncovered another bug. Done in -> aaf12ba

Bonus: annotations and shapes with invalid category coordinates were broken too. Fixed in -> 0758b3b

@alexcjohnson
Copy link
Collaborator

Beautiful. Gotta love it when all these extra edge cases pop out on fixing related issues.
馃拑

@etpinard etpinard merged commit 1c38597 into master Jun 19, 2017
@etpinard etpinard deleted the fix-category-autorange2 branch June 19, 2017 13:57
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.

Annotation autorange on category axes
2 participants