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

Inconsistent markup for floated elements #13861

Closed
ChemicalSailor opened this issue Feb 13, 2019 · 10 comments
Closed

Inconsistent markup for floated elements #13861

ChemicalSailor opened this issue Feb 13, 2019 · 10 comments
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.

Comments

@ChemicalSailor
Copy link

ChemicalSailor commented Feb 13, 2019

The image block adds additional markup when float alignment is selected to help with styling as per the handbook:

<div class="wp-block-image">
    <figure class="alignleft">
        <img src="..." alt="" width="200px" />
        <figcaption>Short image caption.</figcaption>
    </figure>
</div>

When floating a pull quote block, the wrapping div is not added which makes markup and therefore styling inconsistent.

<figure class="wp-block-pullquote alignright">
       <blockquote>...</blockquote>
</figure>

A floated blockquote (in fact all floated blocks) should be treated the same in terms of adding extra markup.

@youknowriad
Copy link
Contributor

I know there are valid reasons for this (probably to support existing themes) but I'll defer to @jasmussen as he has more context here.

@jasmussen
Copy link
Contributor

You are correct that the additional markup is to help with styling.

I tried expanding this markup to additional blocks in #9189, but the key difference between the image block and the pullquote block is that images have an intrinsic width, and the pullquote does not.

This doesn't mean it can't be done, but just that it's not as simple as I had hoped it was, as is evident by my comment here: #9189 (comment)

As such, I'd not personally be opposed to expanding that markup to additional blocks. But I'm not entirely sure I agree that all floated blocks should be treated the same. Floats are inherently quite complex, even if they can appear simple on the face of it.

Even though I personally agree it could be worthwhile, simply for the consistency, I have to ask (for my own sake as well): what problem does it solve to expand this markup? It does add more markup where it might not be needed, and the most basic float style implementations (float: left; margin-right: 2em;) should work fine with either markup.

Also CC: @kjellr — can you think of how this kind of change in markup might affect WordPress themes?

@kjellr
Copy link
Contributor

kjellr commented Feb 14, 2019

Also CC: @kjellr — can you think of how this kind of change in markup might affect WordPress themes?

My biggest concern is that moving the alignment class into a child of a wrapper element would break a lot of themes.

In Twenty Nineteen for instance, the default alignment rules specifically target direct children of entry-content. If we were to relocate the alignment class, that'd break the behavior that pushes right alignments outside of the text column. Rules that target things like .wp-block-quote.alignleft would be broken too.

If I'm remembering correctly, the image block was actually the only block that did add this extra wrapper class. We had issues with this special case, and ended up needing to write up new rules to target it correctly.

It's all fixable, and I definitely see the value in being consistent, but I worry about this being a breaking change for themes.

@ChemicalSailor
Copy link
Author

I tried expanding this markup to additional blocks in #9189

I remember reading this a while ago but didn't click what impact it would have. I can see this issue has been raised a few times in various guises.

My background in raising this is in trying to update an old theme to make use of gutenberg features, particularly the alignwide/full classes using the suggested .entry-content > *:not(.alignwide):not(.alignfull) selector.

I've made a simple pen to compare classic editor markup and gutenberg markup which shows how e.g a pullquote block is not floated the same as an image block using minimal CSS. As you can see the issue is that without a wrapper around the "pullquote" float, it floats out to the edge of the container which is not inline with float behaviour of the classic editor or the image block; also the default editor styles keep the floated pullquote inline with the width of the text.

Of course, it's possible to fix this with adding targeted styles, but this kind of feels hacky and not maintainable, especially with 3rd party blocks.

the key difference between the image block and the pullquote block is that images have an intrinsic width, and the pullquote does not.

Am I right in thinking that this is basically already solved by giving floated elements a max-width? In which case it should be possible to apply consistent markup now?

My suggestion would be that each floated block has a wrapping div like the image block, but with a has-float class. That would allow theme authors to keep floats inline with paragraph widths, or break them out by targeting the width has-float. Maybe this can be made opt-in for themes, to help with maintaining backward compatibility.

The other option is that (if images really are just the odd ones out) getting rid of the wrapper around images and let them float out to the edges the same as everything else.

@jasmussen
Copy link
Contributor

I've made a simple pen to compare classic editor markup and gutenberg markup which shows how e.g a pullquote block is not floated the same as an image block using minimal CSS. As you can see the issue is that without a wrapper around the "pullquote" float, it floats out to the edge of the container which is not inline with float behaviour of the classic editor or the image block; also the default editor styles keep the floated pullquote inline with the width of the text.

Yes, I've been down the same rabbit hole. But we had, for a long time, a version of that same structure that worked relatively well, without the wrapping div:

https://codepen.io/joen/pen/oEYVXB?editors=1100

We tried our hardest to not add additional markup, just to try our best to keep the markup clean. But in the end, it was captions on images that proved to be the problem. For the caption to be no wider than the image itself, we had to add a wrapping div:

https://codepen.io/joen/pen/zLWvrW

However for quotes and pullquotes, it should be possible to achieve the same, without the additional div, right? Speaking again to the question of intrinsic width, you kind of have to apply both a min-width and a max-width to the quote block itself, and any citations or other content will be bounded by that. Whereas a figure that is sized by the width of the img inside won't necessarily know what to do with the figcaption below.

Am I right in thinking that this is basically already solved by giving floated elements a max-width? In which case it should be possible to apply consistent markup now?

I'm not entirely sure I'm parsing that sentence above. As mentioned, the key reason for adding the wrapping div around the image, is that images in Gutenberg basically have this structure:

<figure>
	<img .../>
	<figcaption>
</figure>

The image should be responsive, but it should also be resizable. So we can't set a max-width on the figure, we can't even set a width. The figure has to be sized by the image.

What then about the figcaption? How do we ensure that it's now ider than the image? That was the challenge that ultimately led to the wrapping div.

But the previous markup, as shown in this codepen, https://codepen.io/joen/pen/oEYVXB?editors=1100 — that should still work for pullquotes. Right?

The above is not to say it won't ever make sense to make the markup consistent, that feels like a separate and valid discussion to have, regardless of implementation details. The above was just to explain and hopefully help out.

@draganescu draganescu added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks Needs Technical Feedback Needs testing from a developer perspective. labels Mar 13, 2019
@ChemicalSailor
Copy link
Author

@jasmussen Sorry about the delay on feedback. I don't get to look at this stuff full-time. Thanks for linking your pens, and the background explanation, it does help to know where a decision has come from. I think we've got a bit distracted by the width of the component; the real issue is the positioning of floated elements on the frontend, which is an issue regardless of width.

To recap: Using the simplest markup and the selector .entry-content > *:not(.alignwide):not(.alignfull) to set the width of content, a floated image will be aligned with said width, whereas other elements (such as the pullquote) will be positioned to the container width, giving inconsistent presentation.

.entry-content > *:not(.alignwide):not(.alignfull) {
   max-width: 80%;
}

.alignright {
   float: right;
   /* an image is contained by wrapping div with 80% width,
   other floats are positioned to the container (100% width) */
}

I've now worked around this issue in my theme by controlling the margins for floated elements that do not have the additional wrapping div. This is not ideal because it requires redefining these rules for different breakpoints (if changing the content width).

.entry-content > *:not(.alignwide):not(.alignfull) {
   max-width: 80%;
}

.alignright {
   float: right;
}

.entry-content > .alignright {
   margin-right: 10% !important;
   /* un-wrapped floats require extra margin definitions
   in this case (100 - 80) / 2 = 10% */
}

My feeling is that once we have to start defining this for multiple breakpoints the extra CSS is more burdensome than additional markup in the HTML.

I see two possible resolutions:

  • apply alignright / alignleft classes to the wrapping div of the image block instead of the figure, matching the behaviour of other floats. They will always float to the width of .entry-content and will need margins to alter position
  • give all floats a wrapping div, ideally with a class such as has-float so the width of the wrapper can be controlled by the theme author if required.

Once the section block lands, some of this control can be given to the user (either method above works).

As an aside, I looked at how these rules affect classic editor content and it becomes even more convoluted. An image without a caption gets positioned to the text width (because it's wrapped by a paragraph), whereas an image with a caption gets positioned to the container (because the enclosing div.wp-caption has the float).

@jasmussen
Copy link
Contributor

Thank you for the thorough response. I do hear you and the issue makes sense still.

There will be some challenges with keeping the current behavior working for themes that have adapted to what's shipping, not nothing insurmountable. I have a bit on my plate so can't tackle this right now, but let's keep the issue open.

@aduth aduth removed the Needs Technical Feedback Needs testing from a developer perspective. label Apr 22, 2019
@Luehrsen
Copy link
Contributor

Luehrsen commented Apr 24, 2019

I am currently facing the same issue. We are experimenting with content that is alignfull-first, meaning it has no wrapper div with constraining width. Very similar to https://codepen.io/joen/pen/oEYVXB?editors=1100

The current implementation falls apart on blocks like cover or button. We are currently toying with a similar approach to @ChemicalSailor, but we found that this falls apart very fast once you have nesting blocks, like the new Group block.

There must be an official best practice. What is the 'official' recommendation on how to handle these kinds of content? Wrap the content in a container with constraints and apply minus-margin to alignwide and alignfull?

Edit: From what I have seen it seems like the editor does in the backend what is being proposed here: Move the floating into a wrapping div to maintain controllable max-width. I believe this is very much worth addressing now, as we are about to officially ship nested wide/full blocks with the group block. That is where a lot of implementations will fall apart and a lot of edge cases will have to be fixed.

Some more info here: https://wordpress.slack.com/archives/C02QB2JS7/p1556179139465200

@stevenmunro
Copy link

I managed to pull my pull-quote exactly to where I wanted it by using this styling. This may appeal to those who are using multiple max-widths depending on device width.

.entry-content > *:not(.alignwide):not(.alignfull) {
   max-width: 768px;
}

.alignright {
   float: right;
}

.entry-content > .alignright {
  margin-left: 2rem;
  margin-right: calc(50% - 768px/2) !important;
}

@youknowriad
Copy link
Contributor

This is definitely still an issue that makes things harder for theme authors. We're rethinking alignments in #20650 in order to solve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

8 participants