-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix video alignment after dependency update (reverted) #10566
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
Conversation
skyace65
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built this locally and everything looks good!
| * theme does not follow, so we set the `align-default` class instead to avoid | ||
| * collisions and have full control of how videos are aligned. */ | ||
| .align-default { | ||
| text-align: center |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| text-align: center | |
| text-align: left |
If we would rather keep exact parity with existing behavior, we can do this. I chose to center the videos because:
- The original report of this problem on RC implied to me that centering might be the desired behavior (even though in 4.3 our videos were aligned left!)
- Images using the
.. figure:role are generally aligned center, images using the.. image:role are generally aligned left (implicitly). So there is some precedent for center alignment. - I think it looks better :)
I don't really care all that much one way or another. This is something we should perhaps standardize (for images) at some point, along with guidelines about whether/when to use .. figure: or .. image:.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke tables in the class reference (and possibly elsewhere) I think, they're now all centered and it really isn't readable
|
Thanks for doing this! |
|
Reverted in #10610. |
In #10462 we updated our dependency on sphinxcontrib-video, which added the ability to specify alignment for videos in sphinx-contrib/video#40. However, that changed the existing alignment behavior.
In the 4.3 (current stable) branch of the docs, before we bumped the dependency, videos were left-aligned:


In the current latest branch, after we updated the dependency, videos are now default left-aligned, but when placed immediately before a section header, appear next to the header rather than above it:
I believe this is due to some assumptions made by the video plugin. When you set
:align: left(see here), div containing the video has thealign-leftclass, and similarly for:align: centerbecomingalign-center, etc.The theme used by the docs for the plugin does:
While our theme does:
Similarly,
our theme does not actually center when usingour theme's centering usingalign-centeralign-centerdoes not work on the 100% width div used by the video plugin.Our Sphinx theme is mostly not our own, but is based on the Sphinx RTD theme. We could probably align our version of
align-centerandalign-leftwith the assumptions made by the plugin, but that may have side effects. We should perhaps investigate this later.Rejected solution:
Use
:align: center. Since this doesn't usefloat, it doesn't cause a wrapping problem, and it matches the behavior of our 4.3 docs. But it is also still aligned left, while semantically it says "centered".Proposed solution:
Use the
align-default, along with some custom CSS to either left- or center-align, depending on what we think looks better. Currently this PR center-aligns. As best I can tell, this is unused in the Sphinx RTD theme so we can freely set CSS for it without worrying about side effects.I think it's still worth investigating later if we can use either
align-leftoralign-centerinstead, but that would likely involve some CSS changes anyway to override what the default Sphinx RTD theme is doing. So it doesn't really save any technical complexity.Here's what the options look like, with a video placed immediately before a section header. Only the first option uses custom CSS; it is what is implemented in this PR.

(BTW, thanks @markdibarry for catching this while we're still in beta!)