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

Handle invert_axes for all Element types #1919

Merged
merged 12 commits into from Oct 7, 2017
Merged

Handle invert_axes for all Element types #1919

merged 12 commits into from Oct 7, 2017

Conversation

philippjfr
Copy link
Member

We never implemented invert_axes for all Element types. This code does that ensuring that any plot can now be adjoined.

Fixes:

@jlstevens
Copy link
Contributor

Looks good although I was hoping to see cruft removed with a replacement at the level of ElementPlot or some such base class.

Happy to merge once the tests pass, ideally with a few new unit tests.

@philippjfr
Copy link
Member Author

This is not ready to review or merge.

@jlstevens
Copy link
Contributor

This is not ready to review or merge.

I know! Which is why I kept my comments general and checked you hadn't added the WIP tag (yet).

@philippjfr
Copy link
Member Author

Looks good although I was hoping to see cruft removed with a replacement at the level of ElementPlot or some such base class.

Not really possible, a baseclass can't know how to properly invert the data for a particular Element.

@philippjfr
Copy link
Member Author

Ready to merge.

@jlstevens
Copy link
Contributor

I'll merge though I do have one question.

Not really possible, a baseclass can't know how to properly invert the data for a particular Element.

Perhaps this could be made easy at the data API level?

@jlstevens jlstevens merged commit 3b11689 into master Oct 7, 2017
@philippjfr
Copy link
Member Author

If it was just about invert_axes I'd definitely agree with you, but in many cases it's tied up with the code to invert_xaxis and invert_yaxis, which definitely isn't a data API level concept.

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