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

Add centering style also to the theme style output #5706

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

jasmussen
Copy link
Contributor

Fixes comment in #5209 (comment).

Previous implementation assumed the theme would handle centering, just like it's traditionally handled alignleft and alignright classes, but since the aligncenter is now applied to the figure element, any theme that uses code like img.aligncenter won't work. Hence this PR.

Fixes comment in #5209 (comment).

Previous implementation assumed the theme would handle centering, just like it's traditionally handled alignleft and alignright classes, but since the aligncenter is now applied to the `figure` element, any theme that uses code like `img.aligncenter` won't work. Hence this PR.
@jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Mar 20, 2018
@jasmussen jasmussen self-assigned this Mar 20, 2018
@jasmussen jasmussen requested a review from a team March 20, 2018 07:46
@jasmussen jasmussen mentioned this pull request Mar 20, 2018
3 tasks
@aduth aduth added the [Type] Plugin Interoperability Incompatibilities between a specific plugin and the block editor. Close with workaround notes. label Mar 20, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

For images which are resized, the explicit width is assigned to the figcaption, so it won't be properly centered by text-align: center; (since the width of the container itself is restricted). In the theme I'm using (Astra), .aligncenter is styled with display: block; margin-left: auto; margin-right: auto;

It's a bit of a slippery slope to start making these fixes on behalf of themes. How far do we take this? What about themes which have their own .aligncenter (not img-filtered) styles which conflict with the ones we define as the compatibility styles?

@jasmussen
Copy link
Contributor Author

In general I'd be okay with closing this PR as a "wontfix" and let themes do the work.

However the change to add a figure around an img is somewhat big, and given how basic an element images are, it would be nice if images, floats and centering did not require theme updates. So far we've noted that you don't have to change your theme at all, to make it Gutenberg compatible, though you can opt in to wide alignments.

As such, I'd prefer to update this PR with the suggested code you noted, for images specifically. But regardless, I'm pinging @karmatosed for a decision & thoughts.

@karmatosed
Copy link
Member

However the change to add a figure around an img is somewhat big, and given how basic an element images are, it would be nice if images, floats and centering did not require theme updates. So far we've noted that you don't have to change your theme at all, to make it Gutenberg compatible, though you can opt in to wide alignments.

I agree with this and whatever we can do to ease we should. I also agree we don't want to bring too much burden into Gutenberg, it's a balance. However, in this case I feel it's important to add this.

@jasmussen
Copy link
Contributor Author

@aduth I don't disagree with you — it feels slightly like overstepping, to add these styles. However in stock Twenty Sixteen using Gutenberg, it feels like centering this image, unresized, in Gutenberg should just work:

raised-fist_270a

If I apply your suggested styles, (display: block; margin-left: auto; margin-right: auto;), I get this in the editor:

screen shot 2018-03-22 at 11 55 10

and thijs on the frontend:

screen shot 2018-03-22 at 11 54 34

If I add the "text-align center", it works:

screen shot 2018-03-22 at 11 57 51

It even works if I resize the image:

screen shot 2018-03-22 at 11 58 25

It also works with resized and unresized floats:

screen shot 2018-03-22 at 11 59 19

screen shot 2018-03-22 at 12 00 03

I understand your concern, and pushed your suggested code from Astra as well. I share some of those concerns myself in #5209 (comment), I even created this PR to address the float+caption issue in a different way: #5460

What are your thoughts on how best to proceed?

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Let's merge this as-is. I agree figcaption wrapper changes warrant some transitional defaults, also at least scoped to the block.

@jasmussen jasmussen merged commit f00a6e2 into master Apr 4, 2018
@jasmussen jasmussen deleted the fix/image-centering-in-themes-also branch April 4, 2018 06:49
@jasmussen
Copy link
Contributor Author

👍 👍 thanks Andrew.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement. [Type] Plugin Interoperability Incompatibilities between a specific plugin and the block editor. Close with workaround notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants