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

Move "opinionated" Gutenberg block styles to theme.scss #7624

Merged
merged 2 commits into from
Jul 6, 2018

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Jun 29, 2018

Goal: With 3.0, we merged in changes to allow Gutenberg to provide opinionated styles for blocks. For example, to provide CSS styles for the blockquote, which is traditionally styled by the theme itself. Simply put, a theme.scss exists for each block, and a theme can opt out of those styles, indicating a desire to provide these styles itself. See also https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#default-block-styles. Fixes #7172.

This moves opinionated styles, such as colors, fonts, and other non-structural elements to separate theme.scss files which require opt in by the theme itself. This includes in this PR:

  • Code styles
  • Embed captions
  • Image captions
  • Preformatted styles
  • Pullquote fonts & border styles
  • Quote styles
  • Table styles
  • Video captions

In addition to this there's a new caption style mixin which reduces caption style reuse.

@jasmussen jasmussen added [Feature] Extensibility The ability to extend blocks or the editing experience Needs Design Feedback Needs general design feedback. labels Jun 29, 2018
@jasmussen jasmussen self-assigned this Jun 29, 2018
@jasmussen jasmussen requested review from mtias and a team June 29, 2018 08:19
@jasmussen
Copy link
Contributor Author

jasmussen commented Jun 29, 2018

I'm still a bit fuzzy on whether these styles are opt-in or opt-out, so please enlighten me. Noting that in master, opinionated styles for Quote and Separator blocks now live in theme.scss, but they are still loading in the Gutenberg Starter Theme, suggesting that the styles are opt-outable. Perhaps an ingredient is missing, just wanted to note this, as I have no strong opinion either way.

Some further questions:

  • Given Pullquote is a new block, do styles here need to be opt-out-able, or are you automatically opting in given it's a new block?
  • There are a lot of duplicated caption styles that are all the same. Is there a refactor we can do here?
  • Is "preformatted" a block we want to provide opinionated styles on?
  • How about subheading?
  • Table we almost certainly want to provide some opinionated styles on. I can imagine many themes wanting to opt into these as it can be a slog to duplicate the table code from theme to theme, and it would be nice to just let Gutenberg do this. Can a theme opt in or out on a per-block basis?
  • Videos and video embeds in Gutenberg are responsive. Should this responsiveness live in theme.scss?
  • Other blocks to discuss include table, text columns, verse, video

@BinaryMoon
Copy link

You opt into the theme styles with add_theme_support( 'wp-block-styles' ); - this is in the Gutenberg starter theme.

In the editor style.scss, edit.scss, and theme.scss are all used; however I believe (hope) you can dequeue them individually.

In the theme only style.scss is used by default, and theme.scss is opt in.

@BinaryMoon
Copy link

In regards the theme styles - the way I think of it, the styles should be in theme.css if they are styles that would already be styled in most existing themes. For example there should be no font declarations since they would already be set in the theme. And there should be no blockquotes since that should already be designed.

Based on what you have done I would suggest:

  • Code block - should be in theme.scss since themes generally style pre elements.
  • Captions - I suspect themes will already style this so I would add to theme.scss
  • Gallery - It sounds like this is not the default WordPress gallery layout (which themes should already support) so I would leave this in style.scss

You can always have structural bits in style.scss and visual styles in theme.scss

@BinaryMoon
Copy link

Given Pullquote is a new block, do styles here need to be opt-out-able, or are you automatically opting in given it's a new block?

It depends on the markup.

Do pullquotes use alignleft and alignright? Those are required items for themes on wordpress.org so themes should already account for a lot of the positional styles. You should then move the visual styles (borders and font sizes) into theme.scss so that they don't conflict with theme defaults.

Related to that, given the recent improvements to block variations (#7362), do we retire the Pullquote block and make it a variation of the Quote block? (#5947)

Personally I would prefer that pull quote gets removed. I'm not sure the average user will know the difference between a block quote and a pull quote.

There are a lot of duplicated caption styles that are all the same. Is there a refactor we can do here?
Is "preformatted" a block we want to provide opinionated styles on?

I haven't looked at the code but I think consistency is super important.

How about subheading?

How does a subheader differ from a heading set to h3/ h4? I don't know the answer and I'm a web designer. How will a non-webdesigner know the answer?

Table we almost certainly want to provide some opinionated styles on. I can imagine many themes wanting to opt into these as it can be a slog to duplicate the table code from theme to theme, and it would be nice to just let Gutenberg do this. Can a theme opt in or out on a per-block basis?

AFAIK Only opt in for everything currently. What styles need copying across?

What styles should be opinionated on tables? Do themes not include table styles normally (mine do)?

Videos and video embeds in Gutenberg are responsive. Should this responsiveness live in theme.scss?

I currently use jetpack responsive videos. Is this no longer needed? I know it's a separate topic but as a user I find it confusing that there's so many video embeds - I keep adding the video block and then remebering I have to add the Youtube block. I miss just being able to paste a video url and have it work.

Other blocks to discuss include table, text columns, verse, video

Text columns should be included in style.scss since they aren't currently available.

@jasmussen
Copy link
Contributor Author

jasmussen commented Jun 29, 2018

Solid feedback, Ben, thank you.

the way I think of it, the styles should be in theme.css if they are styles that would already be styled in most existing themes.

I think that's a good rule of thumb, and important to balance this given back compat with themes. This is why I'm asking on a per-block basis. I don't have strong opinions here myself, but want to make sure that any benefits Gutenberg can offer are offered.

For example, I totally agree that the stock blockquote is theme territory unless the theme specifically says "sure, lay Gutenberg styles on me". However we might want to offer the additional quote variations even without opt-in — otherwise a user might pick the 2nd ("Large") quote style only to see no change on the front-end.

Similarly, in Gutenberg an image that is not inserted as an inline image comes with new figure markup and accompanying figcaptions. Do we offer those caption styles by default or should they require opt-in?

Personally I would prefer that pull quote gets removed. I'm not sure the average user will know the difference between a block quote and a pull quote.

I agree, and this circles back to quote style variations again — a pullquote style should probably have at least some structural visuals offered, like a font size, even if a theme hasn't necessarily opted in.

How does a subheader differ from a heading set to h3/ h4? I don't know the answer and I'm a web designer. How will a non-webdesigner know the answer?

The subheading block is a weirdly controversial one. I don't quite understand its purpose myself, and it seems half want it removed, while for the other half it's a make or break block. I don't have strong opinions here myself. There's a ticket somewhere discussing this. I read it as "Dr. Strangelove" being the heading, and "Or: How I Learned to Stop Worrying and Love the Bomb" as the subheading.

AFAIK Only opt in for everything currently. What styles need copying across?

No strong opinions here. I was thinking border-width and border-collapsing. I tried to make it responsive but failed, this might come in a separate block.

I currently use jetpack responsive videos. Is this no longer needed?

Possibly?

embeds

I miss just being able to paste a video url and have it work.

Is this not working for you? It's working fine for me. Try pasting https://www.youtube.com/watch?v=tEJpLDEOivA on a new line. The Video block is for videos you upload yourself, or direct links to video URLs. The embed blocks are just aliases for every existing WordPress supported oEmbed.

Thanks again Ben, I hope to address all your thoughts in this PR — possibly today, otherwise next week. We should be able to get these in 3.3. Feel free to lay other concerns on me, if there are frustrations I can address, you know where to find me.

@jasmussen jasmussen force-pushed the try/move-opinionated-styles branch from 4dde8cf to 57787fd Compare June 29, 2018 14:35
@jasmussen
Copy link
Contributor Author

Okay @BinaryMoon, I took a stab at your feedback. Please take a look at the diff and see if you feel it addresses your thoughts. Thank you.

57787fd

@samikeijonen
Copy link
Contributor

@BinaryMoon I tried to dequeue theme.css but couldn't do it with wp_dequeue_style( 'wp-core-blocks-theme' );

Tried enqueue_block_editor_assets and enqueue_block_assets hooks. Let me know if you already know how to do it.

@jasmussen
Copy link
Contributor Author

@samikeijonen If you simply don't add add_theme_support( 'wp-block-styles' ); to your theme functions.php, no "opinionated" styles should be loaded for you. Especially after this PR gets merged, I suppose — it's still early days in master. See also https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#default-block-styles

@jasmussen jasmussen changed the title Experiment: Move some styles to theme.scss Move "opinionated" Gutenberg block styles to theme.scss Jun 29, 2018
@jasmussen
Copy link
Contributor Author

Ticket is thoroughly squashed and rebased since the first attempt. So the diff should be easy to follow. I've also renamed the ticket to represent that this is no longer an experiment.

@samikeijonen
Copy link
Contributor

@jasmussen theme.css is always loading in the editor. That's what I try to dequeue.

@jasmussen
Copy link
Contributor Author

jasmussen commented Jun 29, 2018

Ah, understood. Yes it seems like if you intend to provide your own styles for both places, it could make sense to let you control that. However it seems like whereas these styles should be opt-in by the theme, they should be opt-out for the editor, right?

@jorgefilipecosta jorgefilipecosta removed the request for review from a team June 30, 2018 11:17
@jasmussen jasmussen added this to the 3.2 milestone Jul 2, 2018
@jasmussen jasmussen requested a review from a team July 2, 2018 08:29
@jasmussen
Copy link
Contributor Author

@BinaryMoon Ben is there any chance you can take a look at this PR again? I know you're a busy man, but I would very much love to make the next version of Gutenberg easier for you to style. ❤️

@BinaryMoon
Copy link

@jasmussen - Hi Joen - I didn't realise you were waiting for me. Sorry about that.

I think the changes you have suggested help quite a bit, but they're not perfect.

  • code - looks good
  • embed - needs the figcaption removed from styles.scss - it's currently in both theme.scss and styles.scss
  • gallery - should the figcaption use the caption mixin? Plus you should remove the duplicate styles from the gallery style.scss. They should be in style.scss or theme.scss - not both.
  • image - perhaps not the right place to ask this but why is max-width set to none here? This breaks responsive behaviours.
  • preformatted - probably don't need the white space rule since themes should include that, but I guess it doesn't matter. Otherwise looks good.
  • pullquote - why are line height and font size being set for paragraphs? Shouldn't they inherit the themes styles?
  • quote - I think everything from quote should be in theme.scss (as per my pull request). Blockquotes should already have margins and padding set, and font styles should also follow theme standards. Regarding the text-align:right in the quote footer... does Gutenberg take care of rtl styles?
  • table - looks good

@jasmussen
Copy link
Contributor Author

Thank you for the review again. I'm not waiting on you, I just appreciate your feedback as I know how deep into this you are.

I will take another stab at this and fix as soon as I can.

@jasmussen
Copy link
Contributor Author

Thanks Ben. Pushed some changes:

  • Removed figcaption from embed.
  • Removed the theme.scss for the gallery entirely. Since it's a brand new block with unique markup, arguably all the CSS is structural.

To your questions:

should the figcaption use the caption mixin

No, because it's sort of a unique overlay caption style here.

perhaps not the right place to ask this but why is max-width set to none here? This breaks responsive behaviours.

The history of that decision lives in #6496. But the short of it is that when an image is resized, we use the figure element to define the width of the element so that the figcaption could have the same width as the image. However I see your point, and I will make a note to revisit this, though I'd like to do that separately.

preformatted - probably don't need the white space rule since themes should include that, but I guess it doesn't matter. Otherwise looks good.

Not entirely sure I got this, as the rule is primarily for use within the editor. Were you suggesting this rule should've been in style or theme instead?

pullquote - why are line height and font size being set for paragraphs? Shouldn't they inherit the themes styles?

No strong opinion here, but given this is an entirely new block (and by the way possibly one that will be removed and turned into a quote variation instead), I felt that this needed some styling in order to be a separate block at all. In this case, the font size of the body text is rather large, even if it's a stock p tag. Given that this block will likely transform into being a variation of the quote (see #5947), okay to skip on this one for now?

quote - I think everything from quote should be in theme.scss (as per my pull request). Blockquotes should already have margins and padding set, and font styles should also follow theme standards. Regarding the text-align:right in the quote footer... does Gutenberg take care of rtl styles?

As is in this branch right now, every stock standard blockquote is styled by the theme. All that's left in style.css is for the "large" variation of the quote:

screen shot 2018-07-04 at 13 40 16

And even then, only the font sizes to ensure it's a variation at all.

This touches sort of on an issue here — I completely understand why you as a theme developer would not want this, because I trust you to provide your own CSS for large variations.

However for the millions of existing themes out there that do not provide these classes, a user will use Gutenberg to use a large variation of the quote style, and not see any change reflected in the content. What's the best solution here?

For example, the separator block also just received variations:

screen shot 2018-07-04 at 13 43 30

Personally, I feel like it's a good trade-off for Gutenberg to leave the default separator and default blockquote alone, unless a theme explicitly opts into this, but leaving structural CSS for any variations offered. Or should this aspect be discussed as a separate issue?

@BinaryMoon
Copy link

Hey Joen - this is looking a lot better to me.

  • embed 👍
  • gallery 👍

The history of that decision lives in #6496. But the short of it is that when an image is resized, we use the figure element to define the width of the element so that the figcaption could have the same width as the image. However I see your point, and I will make a note to revisit this, though I'd like to do that separately.

I ran into this being a problem using Gutenberg on my personal site on this post. The animations in the sidebar were overlapping the content and wouldn't scale. Happy to offer feedback/ testing when you create a new ticket.

(Re: preformatted) Not entirely sure I got this, as the rule is primarily for use within the editor. Were you suggesting this rule should've been in style or theme instead?

My bad. I thought the code was in theme.scss - I misread the filename.

(Re: pullquote) No strong opinion here, but given this is an entirely new block (and by the way possibly one that will be removed and turned into a quote variation instead), I felt that this needed some styling in order to be a separate block at all. In this case, the font size of the body text is rather large, even if it's a stock p tag. Given that this block will likely transform into being a variation of the quote (see #5947), okay to skip on this one for now?

I'm all for removing pullquote so lets ignore for now.

This touches sort of on an issue here — I completely understand why you as a theme developer would not want this, because I trust you to provide your own CSS for large variations.

That is a challenging one and I totally see your point. The variations is harder to style with default themes.

I wonder if the core code should be changed so that the default variants are only enabled when the default styles are opted in to? That way you can include all the variants in styles.scss and keep theme.scss clean for the rest of us (and we can include our own variants within the themes).

Although that would then raise issues with portability & lock in.

I hadn't seen the separator variations. That's an interesting addition. I think having variants is good - but I can't decide if having quite so many is a good thing. I can imagine this being abused.

jasmussen added a commit that referenced this pull request Jul 5, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
@jasmussen jasmussen mentioned this pull request Jul 5, 2018
@BinaryMoon
Copy link

Let's merge then! It's definitely a step in the right direction.

@jasmussen
Copy link
Contributor Author

Just tested this again, it seems solid and harmless to me. Awaiting @karmatosed call on whether this can make the 3.2 deadline. 🤞

padding: .8em 1.6em;
border: 1px solid $light-gray-500;
border-radius: 4px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Next person to edit this file, if they're using the provided EditorConfig, will introduce a trailing new line to the file.

I'd suggest installing a plugin for your editor so we can be consistent on this:

https://editorconfig.org/#download

Copy link
Contributor Author

Choose a reason for hiding this comment

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

┻━┻ ︵ヽ(`Д´)ノ︵ ┻━┻

I have ESlint installed, and it's not giving me any errors:

screen shot 2018-07-06 at 15 25 12

screen shot 2018-07-06 at 15 25 17

My apologies, and I have added the newline, and now I will not relent unless I get VSCode to behave. This is embarrasing!

Copy link
Member

Choose a reason for hiding this comment

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

Different tool than ESLint 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found it. Turns out that's a VSCode setting that defaults to off, which confused me because I switched from Atom where it defaults to on:

screen shot 2018-07-06 at 15 27 41

I've now fixed this, and made a backup of my user config. Sorry about that.

@karmatosed karmatosed self-requested a review July 6, 2018 13:32
Copy link
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

Looks great to me let's 🚢

@jasmussen jasmussen merged commit 8a03da7 into master Jul 6, 2018
@jasmussen jasmussen deleted the try/move-opinionated-styles branch July 6, 2018 13:34
aduth pushed a commit that referenced this pull request Jul 6, 2018
This PR is a followup to #7624.

It adds back the caption styles for images and embeds. This is because the majority of existing themes won't have styles to accommodate the new `figcaption` markup, and are likely to be styling the WP Captions instead. For those themes, if the caption styles live in the `theme.scss` file, the captions will appear unstyled or broken.

Some time in the future, this can possibly be revisited and shuffled around.
aduth pushed a commit that referenced this pull request Jul 6, 2018
This PR is a followup to #7624.

It adds back the caption styles for images and embeds. This is because the majority of existing themes won't have styles to accommodate the new `figcaption` markup, and are likely to be styling the WP Captions instead. For those themes, if the caption styles live in the `theme.scss` file, the captions will appear unstyled or broken.

Some time in the future, this can possibly be revisited and shuffled around.
jasmussen added a commit that referenced this pull request Jul 31, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 1, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 3, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 6, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 9, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 9, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 10, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 13, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 17, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 20, 2018
This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.
jasmussen added a commit that referenced this pull request Aug 20, 2018
* Revisit image floats.

This adds a wrapping `aside` element to any image block that's floated left or right. From the spec, https://www.w3.org/TR/html52/grouping-content.html#the-figure-element:

> For content that is only tangentially related, or that serves a separate purpose than the surrounding flow, the aside element should be used (and can itself wrap a figure). For example, a pull quote that repeats content from an article would be more appropriate in an aside than in a figure, because it isn’t part of the content, it’s a repetition of the content for the purposes of enticing readers or highlighting key topics.

The above is the reasoning for using the `aside` element to wrap the figure. But why wrap the figure at all?

Because due to issues surfaced in #7624 (comment), it seems our current implmeentation isn't responsive.

The challenge is — what if you float a very small image to the left, and write a giant caption. Even if we apply `width: fit-content;` on the `figure`, the caption will expand the `figure` to accommodate as much text as the parent wrapping element will allow. `min-content` doesn't work either, because this will make the `figure` only as wide as the smallest word.

What we have in master works in most cases, through dark magic, but it also only works because we remove the `max-width` from the nested image. This means the image won't resize with the viewport, and is therefore not responsive.

I have explored so many many options for fixing this, and after all this time, what it boils down to is this:

- We can either set a fixed size on floated captions, say 33%, and hope it works for the image that's floated. Not ideal, and you can see the end result here: https://codepen.io/joen/pen/wXbqwN
- We can add a wrapping element around the `figure`, so that it and the `figcaption` can be sized using table rules, as in this PR.

Why not just set the width on the figure element and float that? Because then we can't accommodate wide images, which rely on an unbounded main column.

It's not ideal that we have to add an extra wrapping element, but it can be semantic, and it feels like the simplest to work with for themers implementing wide images coexisting with floats.

* chore: tweak documentation

* docs: Tweak some comments

Add clarity to HTML elemeents in the comments which might also be read as words.

* fix: Update test fixtures for new image float

* Fix resize handle names

* Revert the part that limits caption width on unresized images

We have a plethora of different combinations of images and captions.

Floated images with captions, non floated images with captions, small images with captions, etc. etc.

In general we use a `display: table;` trick to get the caption to size itself according to the image. However if an unfloated image follows a floated image, this means the unfloated image can size itself down to "fit in the available space", which is not what we want. So for now, if you upload a small image and _don't_ float it, a wide caption will be as centered as the block itself.

This might be worth revisiting, but at this point I'd like to look at that separately from this PR which is already big.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants