Toggle responsiveness for embed blocks #10161

notnownikki · 2018-09-25T13:19:37Z

Description

Added an inspector control to toggle responsive classes on a per block basis.

How has this been tested?

Start a post, embed a youtube video.
Publish the post.
Edit the post, toggle responsiveness to off. Check the responsive classes are removed.
Publish the post. Edit it, and check the classes are still removed.
Toggle the responsiveness back on, publish the post.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

jasmussen · 2018-09-25T13:39:55Z

This is SOLID!

GIF:

It's a little hard to see what the difference is based on that GIF, but it works. Here's the front-end without the Responsive toggle:

In this case, the TwentySeventeen technically makes the video responsive, it's just not as good as the true aspect ratio sensitive responsiveness we made. And it's also going to be on a per-theme basis, obviously.

SO THIS IS WONDERFUL, I think it's a great solution.

Two things. There appears to be an extra bottom margin on the stock iframe that's beeing output:

This pushes the caption down. @kjellr as a theme expert, do you have any insights here on whether this is something Twenty Seventeen adds, or something else? Should we zero it out?

And secondly, we should rephrase the toggle a bit. Right now it says:

Responsive     [on]
Videos are responsive

Responsive     [off]
Toggle to make videos responsive.

This is good, but can be better, especially given the context above where the theme might be providing some tricks. I'm also not sure the casual user knows what "responsive" means. Perhaps we could do:

Scale video to fit     [on]
Videos scale to fit available space

Scale video to fit     [off]
Videos are not scaled to fit available space

Something in that vein?

@michelleweber any insights?

notnownikki · 2018-09-25T13:44:49Z

I'm also not sure the casual user knows what "responsive" means.

Ugh yes, I forgot to do my jargon check. My jargon alarm went off when I wrote the help text, but I didn't follow it up :(

All suggestions welcome!

michelleweber · 2018-09-25T21:47:21Z

Yeah, I'd definitely nix responsive. I think "Videos scale" or something even more descriptive like "Videos automatically resize," if that's not too long, is the way to go.

notnownikki · 2018-09-26T08:36:14Z

I've updated them to "Videos automatically resize" and "Videos are fixed size". Sound good?

jasmussen · 2018-09-26T08:58:29Z

Yep, sound good, and we can always revisit if when this goes out it still isn't clear.

notnownikki · 2018-09-26T09:38:19Z

Great! Would you be ok approving this? I've added it to the 4.0 milestone, it would be nice to get it out there :)

jasmussen · 2018-09-26T09:54:04Z

I agree about the milestone, and because it's there it means it will get code reviews when the time comes. But I expanded the range just to expediate it. Reason being: while I completely trust your code, and the design/experience is 👍 👍 — I don't trust my own skills enough for the code review aspect.

notnownikki · 2018-09-26T09:55:12Z

Sure, makes sense! Thankfully there wasn't too much code to write because of the set of components that Gutenberg provides to make this type of thing easy :)

tofumatt

This seems good to me, I'm just confused about why we prefix a method with a "maybe" but don't say when the "maybe"'s condition is met.

I also think we should get the attribute to align with the help text. But can embeds be something other than video? Should we be saying Allow content to resize rather than Allow video to resize?

Once those things are clarified though this is good to go. Ping me for another review and we can definitely get this in for 4.0 😄

tofumatt · 2018-09-26T12:43:20Z

packages/block-library/src/embed/index.js

 * if the HTML has an iframe with width and height set.
 *
 * @param {string} html The preview HTML that possibly contains an iframe with width and height set.
+ * @return {Object} Object with classnames set to true, for use with `classnames`.


I don't quite understand what this means, I'm guessing it follows this shape:

{ 'is-Mobile': isMobile, 'is-Responsive': isResponsive }, etc.?

Maybe just:

/* * @return {Object} Class names, for use with `classnames()`. */

would work?

tofumatt · 2018-09-26T12:46:03Z

packages/block-library/src/embed/index.js

+ }
+
+ /**
+ * Maybe sets the appropriate CSS class names to enforce an aspect ratio when the embed


Why does this "maybe" set things? I don't know this component very well, but this is a very confusing name/comment to me.

Maybe (:rofl:) I didn't make the comment very clear? It says it the classes if the HTML has an iframe with width and height set. Is there something else that's confusing that I'm missing?

Prefacing it with "Maybe" is confusing. It makes me think it might do something, but I can't tell the reason why it might do it 😄

I think the idea is it sets the classNames if allowResponsive is true.

In that case I think this is best to just be named setAspectRatioClassNameIfResponsive.

Alternatively we could rename it setAspectRatioClassName but I understand why that would be confusing; if allowResponsive is false-y we don't set anything, but the method name implies we are always changing a value.

I just find the "maybe" word confusing and not adding clarity. setAspectRatioClassNameIfResponsive is self-documenting. I think the method documentation could be cleaned up too, because it says stuff about iframe height/width, but in reality the check is around the allowResponsive attribute. We always set the className attribute if allowResponsive is truthy, it's what we set it to that varies on the iFrame props.

What about:

/** * Set the CSS class names to enforce an aspect ratio for an embed * that is allowed to resize. The aspect ratio is calculated from * the iFrame inside the HTML provided. * * @param {string} html The preview HTML that possibly contains an iframe with width and height set. */

Sorry, I know this documentation is tangential to your change but it's quite confusing to me and I think worth tidying up whilst we're changing things here.

If we changed the name of the method like that, it would need to be setAspectRatioClassNameIfResponsiveIsSetAndHTMLContainsIframeWithWidthAndHeight because we don't always set the classnames if allowResponsive is true. We always make the call to set them, but there might be nothing to set.

But yes, the comments needs to be updated to reflect that if allowResponsive is false then the classes will never be set.

But setAttributes is always called if allowResponsive is true.

(Sorry for terseness; I was on mobile when I left that comment!)

As talked about on Slack, I think applyResponsiveClassnames or setResponsiveClassnames would work here and just be a bit clearer/less confusing as a name 😄

Would that be okay?

Sure! The maybe stuff had leaked through from me reading too much oEmbed code and I got slightly corrupted 🤣

I'll get it changed around

packages/block-library/src/embed/index.js

talldan · 2018-09-26T12:52:32Z

packages/block-library/src/embed/index.js

 }
 }
+
+ return {};


I don't think this is needed, as classnames knows to ignore undefined.

notnownikki · 2018-09-26T15:03:40Z

@tofumatt I think I've removed all the uncertainty from those method names now :)

tofumatt

Code looks good to me now, I just have one request for a change of argument name at getAspectRatioClassNames, but I didn't want to make it myself to make sure you're cool with it.

There's also a "Responsive" label still–do we want to change that?

tofumatt · 2018-09-26T15:14:25Z

packages/block-library/src/embed/index.js

 */
- maybeSetAspectRatioClassName( html ) {
+ getAspectRatioClassNames( html, add = true ) {


I'm cool with this approach, but I think this is a bit of a terse variable name. Maybe includeAspectRatioClassnames?

Sorry for all my suggestions being so verbose!

how about allowResponsive? That's the same as the attribute name :)

Works for me! 👍

tofumatt · 2018-09-26T15:15:28Z

packages/block-library/src/embed/index.js

+ <Fragment>
+ <BlockControls>
+ <Toolbar>
+ { ( preview && ! previewIsFallback && <IconButton


I know it was like this when you got here, but this is a bit tough to read; I'm just gonna tweak the formatting.

tofumatt · 2018-09-26T15:15:58Z

packages/block-library/src/embed/index.js

+ <InspectorControls>
+ <PanelBody title={ __( 'Media Settings' ) } className="blocks-responsive">
+ <ToggleControl
+ label={ __( 'Responsive' ) }


This is still labelled as "Responsive", shouldn't the label change?

Oooh, good catch! "Auto-fit content"? "Automatically scale content"?

I like "Automatically scale content" 👍

notnownikki · 2018-09-26T15:44:01Z

@tofumatt better label, better parameter name :) thank you for being so thorough!

Toggle responsiveness for embed blocks

302ad62

notnownikki requested a review from jasmussen September 25, 2018 13:19

notnownikki mentioned this pull request Sep 25, 2018

Potential fix for embed space: move styles to theme.scss #10133

Closed

Better help messages

1f8ee59

Inline doc that explains why we're removing classes when we do

c642378

notnownikki added this to the 4.0 milestone Sep 26, 2018

jasmussen requested a review from a team September 26, 2018 09:53

notnownikki mentioned this pull request Sep 26, 2018

Giant space above all Youtube video in Posts #10109

Closed

tofumatt requested changes Sep 26, 2018

View reviewed changes

Update help text

5405c58

talldan reviewed Sep 26, 2018

View reviewed changes

notnownikki added 2 commits September 26, 2018 15:22

Clear up method names and docs

acf7811

Simplify toggling responsive classes

0441b48

tofumatt self-requested a review September 26, 2018 15:08

chore: Tweak indents of component

be80cc9

tofumatt approved these changes Sep 26, 2018

View reviewed changes

Better parameter name, better toggle label

974fb8d

notnownikki merged commit 46d0f57 into master Sep 26, 2018

youknowriad deleted the update/embed-block-optional-responsiveness branch September 26, 2018 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggle responsiveness for embed blocks #10161

Toggle responsiveness for embed blocks #10161

notnownikki commented Sep 25, 2018

jasmussen commented Sep 25, 2018

notnownikki commented Sep 25, 2018

michelleweber commented Sep 25, 2018

notnownikki commented Sep 26, 2018

jasmussen commented Sep 26, 2018

notnownikki commented Sep 26, 2018

jasmussen commented Sep 26, 2018

notnownikki commented Sep 26, 2018

tofumatt left a comment

tofumatt Sep 26, 2018

tofumatt Sep 26, 2018

notnownikki Sep 26, 2018

tofumatt Sep 26, 2018

notnownikki Sep 26, 2018

tofumatt Sep 26, 2018

tofumatt Sep 26, 2018

notnownikki Sep 26, 2018

talldan Sep 26, 2018

notnownikki commented Sep 26, 2018

tofumatt left a comment

tofumatt Sep 26, 2018

notnownikki Sep 26, 2018

tofumatt Sep 26, 2018

tofumatt Sep 26, 2018

tofumatt Sep 26, 2018

notnownikki Sep 26, 2018

tofumatt Sep 26, 2018

notnownikki commented Sep 26, 2018

Toggle responsiveness for embed blocks #10161

Toggle responsiveness for embed blocks #10161

Conversation

notnownikki commented Sep 25, 2018

Description

How has this been tested?

Types of changes

Checklist:

jasmussen commented Sep 25, 2018

notnownikki commented Sep 25, 2018

michelleweber commented Sep 25, 2018

notnownikki commented Sep 26, 2018

jasmussen commented Sep 26, 2018

notnownikki commented Sep 26, 2018

jasmussen commented Sep 26, 2018

notnownikki commented Sep 26, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented Sep 26, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented Sep 26, 2018