Skip to content

ENH: Don't center the HTML animation rendering (Fixes #11795) #11816

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

ENH: Don't center the HTML animation rendering (Fixes #11795) #11816

wants to merge 1 commit into from

Conversation

dopplershift
Copy link
Contributor

@dopplershift dopplershift commented Aug 6, 2018 •
edited
Loading

Centering the animations makes them align differently than the regular
images in the notebook. Also, align attribute is somewhat deprecated.

This adjusts some of the styling as well so that the controls are
somewhat aligned with themselves.

Closes #11795 .

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak
jklymak approved these changes Aug 10, 2018
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I didn't really check the control alignment, but will take your word for it that its an improvement. But I agree that not centring the animations we create makes sense (downstream user should do so when they embed them).

@timhoffm
Copy link
Member

I've hacked the CSS changes into the first animation of https://splines.readthedocs.io/en/latest/bezier.html:

grafik

The result is not quite convincing:

  • The size of the slider is significantly shorter than the image. In general, since image sizes may vary, a left-aligned slider will always look a bit out of place.
  • The margin-left: 50px creates a pseudo-alignment of the radio buttons, which in general will not work.

IMO the best solution ist to center-align all the controls with respect to the image and left-align the bounding div. This can be achieved e.g. by <div class="animation" align="center" style="display: inline-block">:

grafik

That would basically be sufficient. Not a CSS guru, but as propsed in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/div#Attributes you may additionally replace the align attibute by CSS Grid or CSS Flexbox if you want to get rid of it.

@dopplershift
Copy link
Contributor Author

I'm ok with that. Honestly, I just didn't have the time or will to look up the proper CSS, so thanks for doing it for me. 😁

@timhoffm
Copy link
Member

Ok. Do you update your commit?

@dopplershift
Copy link
Contributor Author

I will.

@dopplershift
Copy link
Contributor Author

Ok, updated. I also took the opportunity to replace the "deprecated" align with margin:auto.

@timhoffm
Copy link
Member

You added margin:auto but contrary to your last comment align is still there. Is that intentional? Also can you please use a consistent style of spacing in the style tag (minor thing).

@dopplershift
ENH: Don't center the HTML animation rendering (Fixes matplotlib#11795)
7afb3b0 
Centering the animations makes them align differently than the regular
images in the notebook. Also, align attribute is somewhat deprecated.

This changes to use inline-block with a div to put the whole animation
at the left, and then center the controls within that block.
@dopplershift
Copy link
Contributor Author

Good catch. Fixed. Actually revealed a problem where align was compensating for my not-quite-sufficient CSS.

@timhoffm
Copy link
Member

Sorry to bother again. For some reason the slider is not centered with this code.

grafik

@timhoffm timhoffm mentioned this pull request Sep 11, 2018
Merged
@dstansby
Copy link
Member

Superseeded by #12098 I think? Feel free to re-open if I'm wrong.

@dstansby dstansby closed this Sep 13, 2018
@dopplershift dopplershift deleted the fix-11795 branch October 8, 2018 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
topic: animation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dopplershift @timhoffm @dstansby @jklymak