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

Toggle responsiveness for embed blocks #10161

Merged
merged 8 commits into from Sep 26, 2018

Conversation

@notnownikki
Copy link
Member

@notnownikki notnownikki commented Sep 25, 2018

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
Copy link
Contributor

@jasmussen jasmussen commented Sep 25, 2018

This is SOLID!

GIF:

embed toggle

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:

screen shot 2018-09-25 at 15 32 50

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:

screen shot 2018-09-25 at 15 33 40

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
Copy link
Member Author

@notnownikki notnownikki commented Sep 25, 2018

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
Copy link

@michelleweber michelleweber commented Sep 25, 2018

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
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

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

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Sep 26, 2018

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

@notnownikki notnownikki added this to the 4.0 milestone Sep 26, 2018
@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

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 jasmussen requested a review from WordPress/gutenberg-core Sep 26, 2018
@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Sep 26, 2018

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
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

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 :)

Copy link
Member

@tofumatt tofumatt left a comment

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 😄

* 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`.

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

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?

}

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

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

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

This comment has been minimized.

@notnownikki

notnownikki Sep 26, 2018
Author Member

Maybe (🤣) 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?

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

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.

This comment has been minimized.

@notnownikki

notnownikki Sep 26, 2018
Author Member

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.

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

But setAttributes is always called if allowResponsive is true.

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

(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?

This comment has been minimized.

@notnownikki

notnownikki Sep 26, 2018
Author Member

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

}
}

return {};

This comment has been minimized.

@talldan

talldan Sep 26, 2018
Contributor

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

@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

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

@tofumatt tofumatt self-requested a review Sep 26, 2018
Copy link
Member

@tofumatt tofumatt left a comment

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?

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

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

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!

This comment has been minimized.

@notnownikki

notnownikki Sep 26, 2018
Author Member

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

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

Works for me! 👍

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

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

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.

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

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

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

This comment has been minimized.

@notnownikki

notnownikki Sep 26, 2018
Author Member

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

This comment has been minimized.

@tofumatt

tofumatt Sep 26, 2018
Member

I like "Automatically scale content" 👍

@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

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

@notnownikki notnownikki merged commit 46d0f57 into master Sep 26, 2018
2 checks passed
2 checks passed
codecov/project 48.81% (-0.02%) compared to af3153a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@youknowriad youknowriad deleted the update/embed-block-optional-responsiveness branch Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.