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 documentation about floats, and possibly expand method #9189

Merged
merged 4 commits into from Aug 24, 2018

Conversation

Projects
None yet
3 participants
@jasmussen
Contributor

jasmussen commented Aug 21, 2018

This branch was started with the intent of porting the float improvements we recently made to images, to other blocks that can be floated, such as Cover Image, Gallery and Embeds.

However as I started doing that, I noticed a lot of different hard-to-genericize aspects of those other blocks. For example, Gallery, Cover Image do not have intrinsic widths, so we have to apply a width to them when they are floated.

For many embeds this is the same, some are resized to their minimum dimensions, such as a tweet that's embedded. Videos are respnosive, however, and rely on an aspect ratio to scale a percentage, so even if they might have an intrinsic width (like 1920px for HD) we scale that down based on the aspect ratio. Which means we have to treat those as if they do not have intrinsic widths either.

Mostly the big change is that the new floats require an extra wrapping div in order to work. I.e.

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

It doesn't have to be a figure, and the alignleft class can be applied to the parent wrapper instead of the figure.

But if we are to use the same float technique, regardless of intrinsic widths, I have to add an additional containing element when the block is floated. Right now this is done for the image: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/index.js#L227

However if the "floats" should be truly generic and work for any block that opts into the float mechanic, we can't implement that wrapping element on a per-block basis.

What are your thoughts on how to generically add this wrapping element for all floated blocks?

Also this PR does two other things:

  1. It adds documentation for how the new markup can be leveraged to style themes
  2. It adds clear: both; to the default appender — it simply does not scale to be functional when it doesn't clear.
Add documentation about floats, and possibly expand method
This branch was started with the intent of porting the float improvements we recently made to images, to other blocks that can be floated, such as Cover Image, Gallery and Embeds.

However as I started doing that, I noticed a lot of different hard-to-genericize aspects of those other blocks. For example, Gallery, Cover Image do not have intrinsic widths, so we have to apply a width to them when they are floated.

For many embeds this is the same, some are resized to their minimum dimensions, such as a tweet that's embedded. Videos are respnosive, however, and rely on an aspect ratio to scale a percentage, so even if they might have an intrinsic width (like 1920px for HD) we scale that down based on the aspect ratio. Which means we have to treat those as if they do not have intrinsic widths either.

Mostly the big change is that the new floats require an extra wrapping div in order to work. I.e.
```
<div class="wp-block-image">
	<figure class="alignleft">
		<img src="..." alt="" width="200px">
		<figcaption>Short image caption.</figcaption>
	</figure>
</div>
```

It doesn't have to be a figure, and the `alignleft` class can be applied to the parent wrapper instead of the figure.

But if we are to use the same float technique, regardless of intrinsic widths, I have to add an additional containing element when the block is floated. Right now this is done for the image: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/index.js#L227

However if the "floats" should be truly generic and work for any block that opts into the float mechanic, we can't implement that wrapping element on a per-block basis.

What are your thoughts on how to generically add this wrapping element for all floated blocks?

Also this PR does two other things:

1. It adds documentation for how the new markup can be leveraged to style themes
2. It adds `clear: both;` to the default appender — it simply does not scale to be functional when it doesn't clear.

@jasmussen jasmussen self-assigned this Aug 21, 2018

@jasmussen jasmussen requested review from tofumatt and youknowriad Aug 21, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 21, 2018

Member

This is labelled as "in progress" but is flagged for review–is this set for review or not ready yet? 😄

Member

tofumatt commented Aug 21, 2018

This is labelled as "in progress" but is flagged for review–is this set for review or not ready yet? 😄

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 21, 2018

Contributor

You are SO FAST HOLY GUACAMOLE!

It's clearly not ready for review. But I would like your opinion on it. I seem to recall in the past that you asked me to flag you for review rather than at mention you in comments because those had a tendency to be lost? I'd be totally fine with un-requesting you for reviews if that's better :D

Contributor

jasmussen commented Aug 21, 2018

You are SO FAST HOLY GUACAMOLE!

It's clearly not ready for review. But I would like your opinion on it. I seem to recall in the past that you asked me to flag you for review rather than at mention you in comments because those had a tendency to be lost? I'd be totally fine with un-requesting you for reviews if that's better :D

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Aug 21, 2018

Member

Haha no worries. All good, just wasn't sure if I would be annoyingly comment on something half-ready! Leave me flagged for review, all good! 👍

Member

tofumatt commented Aug 21, 2018

Haha no worries. All good, just wasn't sure if I would be annoyingly comment on something half-ready! Leave me flagged for review, all good! 👍

@webmandesign

This comment has been minimized.

Show comment
Hide comment
@webmandesign

webmandesign Aug 22, 2018

Contributor

Great to see this being discussed! I thought things are set in stone in terms of block HTML by now…

Affected blocks

…to images, to other blocks that can be floated, such as Cover Image, Gallery and Embeds.

I think Pullquote is another one to be added to the list.

The width

For example, Gallery, Cover Image do not have intrinsic widths, so we have to apply a width to them when they are floated.

This is fine with me. I think there is no other way around. Themes can always tweak those floated widths with their CSS too.

The HTML

But why not modifying the markup of the extra wrapping div by additional align-wrap align-wrap-left classes:

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

That way we would be able to target the wrapping div with the CSS if we (or theme) need to do so for whatever reason.

Alignment class on parent wrapper?

It doesn't have to be a figure, and the alignleft class can be applied to the parent wrapper instead of the figure.

I think we should leave the actual alignment class (such as alignleft) applied on the element container that should actually float (correctly, the figure in above example). Not on the parent wrapper.

The reason for this would be that if a theme styles alignment classes generically, and then some of the blocks break the expected alignment class application by adding it on a parent wrapper, the theme would need to provide also styles like .wp-block-image.alignleft figure {...} which is not manageable for all blocks (core or added by plugins). (Note that using simplified .alignleft > * {...} is not desirable here as what about the blocks that do not apply the extra wrapper?)

Leaving the align class applied on an element that should actually float prevents this issue and the theme can simply style .alignleft {...} only.

Besides, here comes the wrapper class advantage as we can simply use .align-wrap .alingleft { max-width: 50%; } to set the width of such aligned block.

That's my 2 cents :)

Contributor

webmandesign commented Aug 22, 2018

Great to see this being discussed! I thought things are set in stone in terms of block HTML by now…

Affected blocks

…to images, to other blocks that can be floated, such as Cover Image, Gallery and Embeds.

I think Pullquote is another one to be added to the list.

The width

For example, Gallery, Cover Image do not have intrinsic widths, so we have to apply a width to them when they are floated.

This is fine with me. I think there is no other way around. Themes can always tweak those floated widths with their CSS too.

The HTML

But why not modifying the markup of the extra wrapping div by additional align-wrap align-wrap-left classes:

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

That way we would be able to target the wrapping div with the CSS if we (or theme) need to do so for whatever reason.

Alignment class on parent wrapper?

It doesn't have to be a figure, and the alignleft class can be applied to the parent wrapper instead of the figure.

I think we should leave the actual alignment class (such as alignleft) applied on the element container that should actually float (correctly, the figure in above example). Not on the parent wrapper.

The reason for this would be that if a theme styles alignment classes generically, and then some of the blocks break the expected alignment class application by adding it on a parent wrapper, the theme would need to provide also styles like .wp-block-image.alignleft figure {...} which is not manageable for all blocks (core or added by plugins). (Note that using simplified .alignleft > * {...} is not desirable here as what about the blocks that do not apply the extra wrapper?)

Leaving the align class applied on an element that should actually float prevents this issue and the theme can simply style .alignleft {...} only.

Besides, here comes the wrapper class advantage as we can simply use .align-wrap .alingleft { max-width: 50%; } to set the width of such aligned block.

That's my 2 cents :)

@webmandesign

This comment has been minimized.

Show comment
Hide comment
@webmandesign

webmandesign Aug 22, 2018

Contributor

Also, like I've explored in my study, wouldn't it be beneficial to wrap full/wide aligned items too? However, I understand if this is overkill as I can still apply such wrapper in a theme itself.

Contributor

webmandesign commented Aug 22, 2018

Also, like I've explored in my study, wouldn't it be beneficial to wrap full/wide aligned items too? However, I understand if this is overkill as I can still apply such wrapper in a theme itself.

@tofumatt

I made a few tweaks but this makes sense. First pass seems good; feel free to flag me for another review when you think this is ready for another look 😄

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 22, 2018

Contributor

@webmandesign Thanks for your thoughts.

I think Pullquote is another one to be added to the list.

We might retire the pullquote, though: #5947

That way we would be able to target the wrapping div with the CSS if we (or theme) need to do so for whatever reason.

I think @tofumatt and @youknowriad mentioned this as well, applying the alignment class not to the figure but to the containing class. I'm either way on this, but would appreciate thoughts.

I also think I'd need advice on how to actually proceed with this PR, I don't know that I have the skill to take it that much further. Notably:

But if we are to use the same float technique, regardless of intrinsic widths, I have to add an additional containing element when the block is floated. Right now this is done for the image: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/index.js#L227
However if the "floats" should be truly generic and work for any block that opts into the float mechanic, we can't implement that wrapping element on a per-block basis.
What are your thoughts on how to generically add this wrapping element for all floated blocks?

Contributor

jasmussen commented Aug 22, 2018

@webmandesign Thanks for your thoughts.

I think Pullquote is another one to be added to the list.

We might retire the pullquote, though: #5947

That way we would be able to target the wrapping div with the CSS if we (or theme) need to do so for whatever reason.

I think @tofumatt and @youknowriad mentioned this as well, applying the alignment class not to the figure but to the containing class. I'm either way on this, but would appreciate thoughts.

I also think I'd need advice on how to actually proceed with this PR, I don't know that I have the skill to take it that much further. Notably:

But if we are to use the same float technique, regardless of intrinsic widths, I have to add an additional containing element when the block is floated. Right now this is done for the image: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/image/index.js#L227
However if the "floats" should be truly generic and work for any block that opts into the float mechanic, we can't implement that wrapping element on a per-block basis.
What are your thoughts on how to generically add this wrapping element for all floated blocks?

@webmandesign

This comment has been minimized.

Show comment
Hide comment
@webmandesign

webmandesign Aug 22, 2018

Contributor

@jasmussen

Great to see the quotes blocks getting some polish!

I think @tofumatt and @youknowriad mentioned this as well, applying the alignment class not to the figure but to the containing class.

If I understand correctly, that's exactly what I would like to avoid. It would produce inconsistent alignment classes styling which wouldn't even be 3rd-party-blocks-proof.

Using the image example, I would like to have this:

<div class="align-wrap align-wrap-left">
	<figure class="alignleft">
	...

Not this:

<div class="alignleft">
	<figure>
	...

(Note that I've omit the wp-block-image class as that one can go anywhere I think.)

Contributor

webmandesign commented Aug 22, 2018

@jasmussen

Great to see the quotes blocks getting some polish!

I think @tofumatt and @youknowriad mentioned this as well, applying the alignment class not to the figure but to the containing class.

If I understand correctly, that's exactly what I would like to avoid. It would produce inconsistent alignment classes styling which wouldn't even be 3rd-party-blocks-proof.

Using the image example, I would like to have this:

<div class="align-wrap align-wrap-left">
	<figure class="alignleft">
	...

Not this:

<div class="alignleft">
	<figure>
	...

(Note that I've omit the wp-block-image class as that one can go anywhere I think.)

@webmandesign

This comment has been minimized.

Show comment
Hide comment
@webmandesign

webmandesign Aug 22, 2018

Contributor

@jasmussen And, like you've mentioned, I also would love to see the functionality applied globally somehow, not on per-block basis.

Contributor

webmandesign commented Aug 22, 2018

@jasmussen And, like you've mentioned, I also would love to see the functionality applied globally somehow, not on per-block basis.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 23, 2018

Contributor

At this point, in order to ship this documentation along with the new markup in 3.7, I'm tempted to shelve (and open for separate pull requests) the idea of expanding this markup beyond images. I'd still like to see it, but given other things on my plate I don't think I personally have the bandwidth to make this happen.

Contributor

jasmussen commented Aug 23, 2018

At this point, in order to ship this documentation along with the new markup in 3.7, I'm tempted to shelve (and open for separate pull requests) the idea of expanding this markup beyond images. I'd still like to see it, but given other things on my plate I don't think I personally have the bandwidth to make this happen.

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Aug 23, 2018

@jasmussen jasmussen added this to the 3.7 milestone Aug 23, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 23, 2018

Contributor

Addressed feedback about comments, and I think we should get this into 3.7. Expanding the float markup to the remaining blocks is worth doing, but also worth looking at separately due to the other blocks different nature with no intrinsic width.

Contributor

jasmussen commented Aug 23, 2018

Addressed feedback about comments, and I think we should get this into 3.7. Expanding the float markup to the remaining blocks is worth doing, but also worth looking at separately due to the other blocks different nature with no intrinsic width.

@tofumatt

One question about a comment, but feel free to tweak that and :shipit:

@jasmussen jasmussen merged commit b61845b into master Aug 24, 2018

2 checks passed

codecov/project 50.61% (-0.26%) compared to e2f3120
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/float-polish branch Aug 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment