Added class information for core/embed in the frontend by Luehrsen · Pull Request #4118 · WordPress/gutenberg

Luehrsen · 2017-12-21T10:12:11Z

Description

This piece of code extends the core/embed block to fetch the information provided by the oEmbed endpoint and adds information about the embed type and embed provider to the block wrapper.

<figure class="wp-block-embed-youtube wp-block-embed type-video provider-youtube">
    [...]
</figure>

This should provide precise information about what is being embedded to frontend developers and give them the ability to better style according to embedded content.

Please see #4107 for a more detailed description.

How Has This Been Tested?

This has been tested locally and manually. No automated tests were built or run.

Types of changes

New feature: Descriptive css classes for the frontend rendering of an embed block

Checklist:

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

aduth · 2018-01-02T19:58:06Z

The provider_name variable on line 126 should be sanitised.

By sanitized, do you just mean made safe for use in a class name?

In which case, you might be able to use one or more of Lodash's string methods:

Luehrsen · 2018-01-02T20:07:04Z

@aduth That was exactly what I was looking for! Awesome!

Will update this PR tomorrow.

aduth · 2018-01-02T21:41:26Z

Noting that kebabCase should handle both escaping and dash-delimiting:

_.kebabCase( 'Foo" onerror="javascript:alert(\"hax\")"' )
// "foo-onerror-javascript-alert-hax"

Luehrsen · 2018-01-03T22:20:55Z

I have provided a patch and sanitised the provider name. Thanks @aduth for the help!

At this point I am unsure how to handle the tests. I understand that the fixture html-files provide what the end result should look like, but most of the URLs are not valid oEmbed ressources.

So that is where I'm stuck at the moment.

aduth

At this point I am unsure how to handle the tests. I understand that the fixture html-files provide what the end result should look like, but most of the URLs are not valid oEmbed ressources.

I think it should be enough to run npm run fixtures:regenerate and commit the changes?

aduth · 2018-01-04T21:11:43Z

blocks/library/embed/index.js

We'll want to remove this debugging statement.

Removed in recent commit.

aduth · 2018-01-04T21:11:54Z

blocks/library/embed/index.js

Does kebabCase not handle toLower already?

No, afaik kebabCase handles lowerCase, not toLower, which leads to ugly transformations from YouTube to you-tube.

aduth · 2018-01-04T21:14:25Z

blocks/library/embed/index.js

A nice feature of classnames is passing an object for optionally included class names:

const embedClassName = classnames( 'wp-block-embed', { [ `align${ align }` ]: align, [ `type-${ type }` ]: type, [ `provider-${ providerNameSlug }` ]: providerNameSlug, } );

https://www.npmjs.com/package/classnames

Updated to reflect the optionally included class names syntax.

Luehrsen · 2018-01-04T23:25:46Z

Thank you @aduth for the review! Will implement these changes on the next commit.

I still have issues with the fixtures though. I am running the dev environment locally and through docker, as described here.

Running any of the npm run fixtures: commands gives me an error. I traced it to the ´get-server-blocks.php´ script, which expects a regular wordpress in the folders 'above' the gutenberg folder. By design this doesn't exist in the docker workflow.

Am I missing something?

aduth · 2018-01-05T19:02:08Z

I traced it to the ´get-server-blocks.php´ script, which expects a regular wordpress in the folders 'above' the gutenberg folder.

The script also accepts an ABSPATH environment variable (which I assume can also be passed through npm run fixtures:regenerate) to assign the WordPress folder location. I'm not entirely sure where this would exist in the Docker workflow since I'm not using Docker, but I can try to investigate (cc also @youknowriad).

Luehrsen · 2018-01-06T13:05:26Z

I suspect the current issue is, that npm run fixtures:[...] does not work with the Docker workflow.

Can anyone confirm, that it works? And if so, how? (I already tried to run npm from inside the docker container, but obviously it did not have npm or any of the build requirements installed.)

mtias · 2018-01-09T17:43:00Z

blocks/library/embed/index.js

"align" is generally legacy for us, but for the others, let's try to use the is- and has- convention, which limits potential clashes. (The encouragement being that people would style as .wp-block-embed.is-provider-youtube.

Good proposal. Will implement that with next commit.

mtias · 2018-01-09T17:43:17Z

blocks/library/embed/index.js

This class should be auto-generated.

Technically yes, but this is only true on the core/embed block. If the user chooses one of the flavored embed blocks, e.g. core/embed-youtube, the class will change to wp-block-embed-youtube and the wp-block-embed class will not be available anymore.

You could mitigate that in CSS with unqualified attribute selectors (as described in #4107), but that would be at a huge cost for frontend rendering performance.

So I am currently thinking about how to avoid class duplication on core/embed, the behavior on the flavored embeds is as intended.

Since for a YouTube embed, the wp-block-embed-youtube class is applied, isn't the is-provider-youtube class then redundant?

Non-blocking: We'd probably expect the fixture markup to be different for most embeds, i.e. including providerNameSlug and type attributes / classes:

 <figure class="wp-block-embed-youtube wp-block-embed is-alignright is-type-video is-provider-youtube"> https://www.youtube.com/watch?v=c-WDHG6rrdU </figure> 

The 'is-provider-' classes are needed for two reasons:

We cannot be sure, that the user currently uses the correct block. They can use the wp-block-embed to yield the same result as with wp-block-embed-youtube. That was the whole premise of this PR.

The provider name can be different for some providers like the wordpress embed, where it describes the URL where the content is coming from. That adds another level of description for theme editors. (Like the ability to style the WP Embed block differently by provider.)

As for the fixture markup, how would I approach that? Do I have to add that by hand?

pento · 2018-01-10T02:15:15Z

In #4279, I've changed the fixtures:server-registered script to run in the Docker instance, so that it no longer requires a local WordPress install.

pento · 2018-01-10T03:45:59Z

Oh, in the mean time, you should be able to run this, instead of npm run fixtures:regenerate:

> npm run fixtures:clean
> docker-compose -f docker/docker-compose.yml run -w /var/www/html/wp-content/plugins/gutenberg -e ABSPATH=/usr/src/wordpress/ --rm wordpress ./bin/get-server-blocks.php > blocks/test/server-registered.json
> cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit

Luehrsen · 2018-01-10T08:57:36Z

@pento Awesome, thanks! One quick note: I had to install cross-env globally to make it run.

And I still think I'm doing something wrong with the fixtures.

Anyways, on round of drinks on me in Belgrade.

pento · 2018-01-11T00:45:57Z

Oh, it looks like you don't need to use cross-env, you can just run GENERATE_MISSING_FIXTURES=y npm run test-unit.

The current errors are caused by the fixture files not having the wp-block-embed class - you'll need to add that manually to the core-embed__*.html files, then regenerate the fixtures.

Luehrsen · 2018-01-11T09:20:41Z

@pento Thanks a lot. Fixtures seem to work now.

Travis still complains, but that doesn't seem to be related to the tests, something in npm fails.

pento · 2018-01-12T05:39:49Z

blocks/library/embed/index.js

Plain strings should use single quotes - this is the Travis error you're seeing.

Luehrsen · 2018-01-12T09:48:50Z

Okay, this seems to be what I wanted to do.

With this I have introduced a duplication issue on the core/embed block. (Which now has the wp-block-embed class twice.) This is a result of forcing the wp-block-embedclass onto the child blocks like core-embed/vine and core-embed/youtube.

I would address the issue in another PR, where I add a proper duplication check in the addGeneratedClassName class.

( And thanks to @pento, @aduth and @mtias for your patience with me! )

Luehrsen · 2018-01-22T09:18:23Z

@aduth Any timeline, thoughts or requirements for this PR going to be merged? :)

aduth

Should the fixtures HTML be updated to include examples of where we'd now expect providerNameSlug and type?

Apologies for the delay in re-reviewing this one. I'm away on a work trip this week but will try to keep a closer eye on it.

aduth · 2018-01-23T09:22:04Z

blocks/library/embed/index.js

If we're setting this into attributes, is there a need on the previous line to set into state as well? Wouldn't this be redundant, and at worst potentially fall out of sync?

From what I have tested we actually need both. The state delivers the html, while the attributes deliver stuff that is needed on save. But I am far away from understanding the inner workings of react.

Noting that because this sets attributes, when loading an existing post which had contained the previous embed block, it will immediately become marked as "dirty" (i.e. needing save) because its attributes have changed.

I guess this would be expected though.

aduth · 2018-01-23T09:23:49Z

blocks/test/fixtures/core-embed__vine.html

I think these Vine files were inadvertently reintroduced after being removed in #4521.

Merging the master branch into my PR branch should fix it, or should it not?

No, I think it will re-add the files when merged.

I have deleted them by hand now. That should do the trick.

aduth · 2018-01-23T09:24:26Z

package-lock.json

I wouldn't expect there should be any changes to this file if we're not updating package.json.

Luehrsen · 2018-02-01T13:28:51Z

@aduth can you take a quick look at the recent changes and merge if it fits? :)

aduth

The permissions changes on package-lock.json are unexpected.

aduth · 2018-02-01T20:58:13Z

blocks/library/embed/index.js

Since for a YouTube embed, the wp-block-embed-youtube class is applied, isn't the is-provider-youtube class then redundant?

aduth · 2018-02-01T20:59:25Z

blocks/library/embed/index.js

Non-blocking: We'd probably expect the fixture markup to be different for most embeds, i.e. including providerNameSlug and type attributes / classes:

 <figure class="wp-block-embed-youtube wp-block-embed is-alignright is-type-video is-provider-youtube"> https://www.youtube.com/watch?v=c-WDHG6rrdU </figure> 

Luehrsen · 2018-02-02T09:38:48Z

@aduth @pento I might need help with the package-lock.json

I don't think I'm able to remove it from the PR, but I copied the current package-lock.json from origin/master by hand. Is there anything else I can do?

aduth · 2018-02-02T13:46:48Z

As for the fixture markup, how would I approach that? Do I have to add that by hand?

The .html (not .serialized.html) file in blocks/test/fixtures for each block is the only one which needs to be managed manually. The rest are automatically regenerated with the npm run fixtures:regenerate command. Again, this is not strictly necessary, though if we expect these blocks to be saved with that particular markup, even if optional, might be good to update the fixtures to reflect this.

I might need help with the package-lock.json. I don't think I'm able to remove it from the PR, but I copied the current package-lock.json from origin/master by hand.

Based on the contents, you might have copied an older version? I'd expect copying the contents from here and committing them should resolve any changes:

https://raw.githubusercontent.com/WordPress/gutenberg/master/package-lock.json

Otherwise, if you give me commit access to your fork, I could try to resolve it.

Luehrsen · 2018-02-10T19:05:08Z

@aduth tried a last time by updating from hand from master.

If it is still out of sync, you should have access to my fork. :)

Greetings from Dubai!

Luehrsen · 2018-02-19T10:40:26Z

@aduth Any chance of this getting some attention? Thanks!

aduth

How should we plan to document for developers to apply styles to an embed block, if they must choose between .wp-block-embed-youtube and .is-provider-youtube?

I rebased your branch and force-pushed to your repository, to make sure it's up to date with latest changes. After verifying changes, you'll want to delete your local copy of your branch, git fetch, and git checkout anew.

aduth · 2018-02-19T16:40:21Z

blocks/library/embed/index.js

Noting that because this sets attributes, when loading an existing post which had contained the previous embed block, it will immediately become marked as "dirty" (i.e. needing save) because its attributes have changed.

I guess this would be expected though.

package-lock.json changes undone

Luehrsen · 2018-02-19T18:08:42Z

As for documentation of the different class types.

wp-block-embed-* gives definite information about what block is being used. This is not a reliable source, due to the fact that multiple values are valid for the same block. E.g.: A Youtube video can be used with wp-block-embed-youtube and wp-block-embed. This class is used to style a specific block.

is-type-* gives general information about the oEmbed type as per oEmbed spec. Valid values are Photo, Video, Link, Rich. This class is used to style a specific group of content. (E.g.: Make all Videos 16:9)

is-provider-* gives specific information about the provider of a piece of embedded content. This is mapped to the provider_name as per oEmbed spec. Please note, that the provider can be very broad, like 'youtube', or very specific, like is-provider-wp-munich for a link like https://www.wp-munich.com/wordpress/first-look-gutenberg-wordpress/.

Examples:
If you want to reliably style all embedded youtube videos: Use the .is-provider-youtube class.
If you want to reliably style all videos to be 16:9: Use the .is-type-video class.

aduth

Looks good 👍 Thanks for your patience.

aduth · 2018-02-24T01:56:23Z

"align" is generally legacy for us, but for the others, let's try to use the is- and has- convention, which limits potential clashes. (The encouragement being that people would style as .wp-block-embed.is-provider-youtube.

@mtias Re-reading this, did you intend for alignment to be kept in legacy format?

It's is- prefixed as of the changes merged here.

I'm finding that, as such, there's a block invalidation in the demo content for embed by alignment:

gutenberg/post-content.js

Lines 129 to 131 in aa1df37

    
           '<!-- wp:embed {"url":"https://vimeo.com/22439234","align":"wide"} -->', 
        
           '<figure class="wp-block-embed alignwide">https://vimeo.com/22439234</figure>', 
        
           '<!-- /wp:embed -->',

We need to decide between:

Revert to legacy format (from is-alignwide back to alignwide)
Update the demo post content

mtias · 2018-02-24T12:32:39Z

@aduth I'd like to update all classes, but it won't be a trivial thing. As it stands, we should probably just use alignwide until we can stop using alignleft, etc.

Luehrsen changed the title ~~Added class information for core/embed in the frontend for #4107~~ Added class information for core/embed in the frontend Jan 4, 2018

aduth reviewed Jan 4, 2018

View reviewed changes

pstonier mentioned this pull request Jan 5, 2018

Add embed wrappers WordPress/gutenberg-starter-theme#29

Merged

mtias reviewed Jan 9, 2018

View reviewed changes

pento reviewed Jan 12, 2018

View reviewed changes

blocks/library/embed/index.js Outdated

Copy link
Copy Markdown

Member

pento Jan 12, 2018

Choose a reason for hiding this comment

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

Plain strings should use single quotes - this is the Travis error you're seeing.

Luehrsen mentioned this pull request Jan 12, 2018

Check for duplication in addGeneratedClassName #4422

Merged

3 tasks

aduth reviewed Jan 23, 2018

View reviewed changes

Luehrsen mentioned this pull request Jan 31, 2018

Embeds: Different blocks/classes on different embed methods #4107

Closed

danielbachhuber added the [Status] In Progress Tracking issues with work in progress label Jan 31, 2018

aduth previously requested changes Feb 1, 2018

View reviewed changes

aduth added [Feature] Blocks Overall functionality of blocks and removed [Status] In Progress Tracking issues with work in progress labels Feb 1, 2018

Added class information for core/embed in the frontend for #4107

39a095b

aduth reviewed Feb 19, 2018

View reviewed changes

aduth approved these changes Feb 19, 2018

View reviewed changes

aduth merged commit 0ad578c into WordPress:master Feb 19, 2018

This was referenced Feb 27, 2018

Blocks: Remove is- prefix from embed alignment class #5293

Merged

Demo: Fix dirtying demo (and subsequent autosave) #5425

Merged

Luehrsen deleted the update/embed-block/css-classes branch August 19, 2018 21:54

mcsf mentioned this pull request Sep 25, 2018

is-video class in the editor, is-type-video class in front-end #10121

Closed

nfmohit mentioned this pull request Oct 14, 2018

[Fix] Establish consistency between front-end and back-end classes for Youtube (Embed) block #10411

Merged

4 tasks

Conversation

Luehrsen commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

aduth commented Jan 2, 2018

Uh oh!

Luehrsen commented Jan 2, 2018

Uh oh!

aduth commented Jan 2, 2018

Uh oh!

Luehrsen commented Jan 3, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luehrsen commented Jan 4, 2018

Uh oh!

aduth commented Jan 5, 2018

Uh oh!

Luehrsen commented Jan 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pento commented Jan 10, 2018

Uh oh!

pento commented Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luehrsen commented Jan 10, 2018

Uh oh!

pento commented Jan 11, 2018

Uh oh!

Luehrsen commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luehrsen commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luehrsen commented Jan 22, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luehrsen commented Dec 21, 2017 •

edited

Loading

pento commented Jan 10, 2018 •

edited

Loading

Luehrsen commented Jan 12, 2018 •

edited

Loading

Luehrsen commented Feb 19, 2018 •

edited

Loading