Add Block Transform to transform Embed blocks into Paragraph blocks. #17413

desaiuditd · 2019-09-12T03:33:47Z

Description

Fixes #15102 by providing a block transform to transform Embeds into Paragraphs.

How has this been tested?

Pasting a URL into the Paragraph block automatically converts into an Embed blocks. After that, now, I'm able to transform the same Embed block into a Paragraph block. Performing an Undo action brings the Embed block back.

While working on this, I found a bug that none of the transformations of core/embed block were applied to core-embed/common/* and core-embed/others/* blocks. So I've fixed that as well.

Screenshots

Types of changes

While converting the Embed into Paragraph, if the caption is present, then I'm using that as the text for the link in the Paragraph. That seemed like a sensible behaviour without the data loss for the user. Happy to defer on someone who's more close to this.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

desaiuditd · 2019-09-12T03:35:17Z

packages/block-library/src/embed/core-embeds.js

+						} );
+					},
+				} ],
+			},


Not sure if this is a breaking change or not. Simply changed the schema of JSON to make it consistent with other transforms.

Same for Speaker Deck block.

desaiuditd · 2019-09-12T03:37:17Z

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

+				transforms: {
+					from: concat( get( transforms, 'from', [] ), get( embedSettings, 'transforms.from', [] ) ).filter( Boolean ),
+					to: concat( get( transforms, 'to', [] ), get( embedSettings, 'transforms.to', [] ) ).filter( Boolean ),
+				},


Basically, apply combined transformations of core/embed as well as other core-embed/common/*

Same for core-embed/other/*

…cks-into-paragraph-block # Conflicts: # packages/e2e-tests/specs/__snapshots__/block-transforms.test.js.snap

paaljoachim · 2020-04-02T07:25:15Z

Hey @desaiuditd

Here is even more information: #21029

Are you able to continue working on these things?

…cks-into-paragraph-block # Conflicts: # packages/block-library/src/embed/core-embeds.js # packages/block-library/src/embed/index.js # packages/e2e-tests/fixtures/block-transforms.js

desaiuditd · 2020-04-05T09:11:57Z

This is good for review again.

paaljoachim · 2020-04-05T10:15:29Z

Hey Udit.

I tested the PR here:
http://gutenberg.run/17413

I then pasted the following links:
https://make.wordpress.org/
https://make.wordpress.org/core/
https://make.wordpress.org/test/2020/03/11/gutenberg-usability-testing-for-january-2020/

All three links brought up a following type of Embed placeholder box:

I clicked to embed but none of them could be embedded.
I clicked to preview the frontend and noticed that all are text links.

Option 1:
Here is my suggestion on how to simplify the process for a user adding a link.

The user pastes inn a link and it automatically shows up as a text link.
As all links can show up as text links.

But some links that can also be embedded. For these special links there can be an option to transform the text link into an embed through for instance the Transform area.

The transform area.

The embed option will only show up where a transform to an embed can happen.

EDIT:
Option 2.
Pasting a link when it is not embeddable automatically adds it as a text link.
Pasting a link when it is embeddable opens a placeholder box to where the user can choose to embed or choose to paste it as a text link. (There also needs to be a simple way to revert to either text or embed link.)

I will ping Miguel, so we can discuss it further here.
@mcsf

desaiuditd · 2020-04-05T10:34:16Z

All three links brought up a following type of Embed placeholder box:

I tried this on preview server as well. But I also noticed these oembed api request failures.

That could be the reason why Gutenberg failed to convert them into embeds on paste.

desaiuditd · 2020-04-05T10:38:58Z

The user pastes inn a link and it automatically shows up as a text link.
As all links can show up as text links.

But some links that can also be embedded. For these special links there can be an option to transform the text link into an embed through for instance the Transform area.

This changes the current default behaviour, which @mcsf mentioned. Some would consider this a breaking change.

It always tries to embed, but this process may fail depending on the source site.

So can we let the current behaviour be as is? And when the process may fail, user has two options to revert. Either transform to paragraph block or click Convert to link button.

…cks-into-paragraph-block

desaiuditd · 2020-04-17T12:36:42Z

I've one question for Option 1 that @paaljoachim has suggested.

The user pastes inn a link and it automatically shows up as a text link.
As all links can show up as text links.

But some links that can also be embedded. For these special links there can be an option to transform the text link into an embed through for instance the Transform area.

how can I check which embed transformation to allow? E.g., if I paste a WordPress Make URL, it’s not probably right to show Youtube embed in the transformation popup, right?

Check this Gif. I got this far. But not sure how to enable only valid embeds out of all. It’s even erroring out for any mismatched embed.

@mcsf Do you have any thoughts on this?

Meanwhile, I will try and see if I can get Option 2 working.

…mbed block.

desaiuditd · 2020-04-18T01:39:43Z

it's better to leave it as is, and avoid abstractions like _mergeTransforms.

@mcsf I was trying to follow DRY because it was same logic for commonEmbeds and otherEmbeds embeds. Also, couple of embeds like Speaker Deck and Crowdsignal were coming with their own transforms. So they needed to be merged with the default transforms. Hence, _mergeTransforms, to retain embed specific transforms as well as the default one.

The core feature to expect from this PR is the ability to convert an embed into a block of text that links to the URL in question.

@mcsf Do you think, I should raise another PR to add smart handling of Embeds as per what Paal is suggesting? That way, this PR can just be about adding new block transform which converts embed into a paragraph with link to the embed url.

paaljoachim · 2020-04-18T08:36:37Z

I think I am just confusing myself here..... I might be running around in circles... sorry about that.
Bottom line is the initial post here: #21029
and @mapk 's response.

I am happy to see that @mcsf Miguel is giving some feedback so we get the concept/development on the right course. Thank you Miguel.

paaljoachim · 2020-04-20T07:12:33Z

So the process should be:
Link -> Can it embed? -> If yes -> Add a placeholder asking if the user wants to embed or convert to text link.
Link -> Can it embed? -> If no -> Automatically add a text link.

@mcsf @desaiuditd @mapk

desaiuditd · 2020-04-20T07:17:22Z

So the process should be:
Link -> Can it embed? -> If yes -> Add a placeholder asking if the user wants to embed or convert to text link.
Link -> Can it embed? -> If no -> Automatically add a text link.

@mcsf @desaiuditd @mapk

Cool. Let me raise another PR to fix this (#21029), so we can have some separation of concern. This PR can fix #15102 in isolation.

…cks-into-paragraph-block

paaljoachim · 2020-04-24T19:10:25Z

I wrote the following in the core editor slack channel a few hours ago and will add it here as well.

So from my understanding of this PR. It takes a part various embeds. Then @desaiuditd will create a follow up PR where embeds are put together even better again. We are looking at all pasted links that embed such as a video etc to work as normal. But pasting a WP post content which normally just adds an embed will instead give the user a placeholder option to either embed the link or to show it as a text link. If someone can check out the PR and let us know if this is the correct approach that would be great. Thanks for any feedback and thoughts shared.

mcsf · 2020-04-27T12:05:33Z

@desaiuditd:

@mcsf I was trying to follow DRY because it was same logic for commonEmbeds and otherEmbeds embeds. Also, couple of embeds like Speaker Deck and Crowdsignal were coming with their own transforms. So they needed to be merged with the default transforms. Hence, _mergeTransforms, to retain embed specific transforms as well as the default one.

Right, the custom transforms for Speaker Deck and Crowdsignal are broken, but are also unnecessary, since — in my testing — the editor corrects the embed type based on the provided patterns anyway. I think we should just remove them, but that belongs in a separate PR.

I appreciate the concern for DRY, although it's a constant tug between DRY and the visual and immediate clarity that sometimes an inline transformation can best provide.

In this case, we don't need to touch any of it, so I recommend reverting.

The core feature to expect from this PR is the ability to convert an embed into a block of text that links to the URL in question.

@mcsf Do you think, I should raise another PR to add smart handling of Embeds as per what Paal is suggesting? That way, this PR can just be about adding new block transform which converts embed into a paragraph with link to the embed url.

Yes, it's best to keep things focused. I personally am not convinced about smart handling of embeds, at least in the way it was it presented, but it can be put up for discussion.

Thanks for coming back to this PR, for making these changes and for keeping things organised!

…cks-into-paragraph-block

desaiuditd · 2020-05-10T05:25:26Z

@mcsf This is good for review again. I have made the requested changes.

mcsf

Thanks for this fix!

paaljoachim · 2020-05-11T13:16:30Z

NB! @mcsf
This PR should not be included into 8.1 which is to be released today.
The PR tears a part embeds and we need the PR that rebuilds embeds to be merged at the same time, so that we do not break Youtube and other embeds.

Bottom line is that adding the test links I have further above in this PR, and a few Youtube URLs none of these can be embedded.

Another example.
Here is one of my dev sites where I added:
https://www.youtube.com/watch?time_continue=4756&v=lSkmtb-OvDk&feature=emb_logo

(The embed)

(Clicking pencil. This is what I see.)

Going to Gutenberg.run and testing this PR.
Here is the result of adding this URL: https://www.youtube.com/watch?time_continue=4756&v=lSkmtb-OvDk&feature=emb_logo

mcsf · 2020-05-11T13:40:34Z

I'm not experiencing any regressions so far. I've compared 81d4f4c (PR head) with the base branch at 601ee4b, and I've tested again with the commit to master at 48aba1e.

You may be experiencing this for being stuck with a stale build in gutenberg.run, something which should be fixed soon (Slack link).

paaljoachim · 2020-05-11T13:53:00Z

Retesting again in gutenberg.run.
I just retested again (in Chrome), and I am getting the same result.

I retested using Firefox, and received the same result.

desaiuditd · 2020-05-11T13:57:29Z

@paaljoachim This PR was never meant to fix the smart auto embedding which you suggested earlier. This PR only allows user to convert an existing embed block into a paragraph block. So it only fixes #15102 in isolation.

The approach you suggested is still being discussed at #21029.

paaljoachim · 2020-05-11T14:16:51Z

Hey @desaiuditd

My concern during the test at gutenberg.run was that one is not able to embed even the Youtube embeds that I tested with.

So from what you are saying (hopefully) the above would be an error.
One should be able to embed. Then take any embed and convert it into a paragraph block.
Is this correct?

ellatrix · 2020-05-11T17:16:41Z

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

+			transform: ( { url, caption } ) => {
+				const link = <a href={ url }>{ caption || url }</a>;
+				return createBlock( 'core/paragraph', {
+					content: renderToString( link ),


Why not write the html as a string?

I don't mind the added peace of mind of running this through the element serialiser. Do you have drawbacks in mind?

jorgefilipecosta · 2020-05-11T17:50:00Z

The PR seems to work well in my tests. I am also able to reproduce the issue @paaljoachim referred but only on gutenberg.run. @aduth verified gutenberg.run has an issue in requests to the endpoint /wp-json/oembed/1.0/proxy?url=.

mcsf · 2020-05-11T17:52:54Z

Thanks for debugging that! Seems we can move on with the release.

paaljoachim · 2020-05-12T14:11:55Z

Testing with 8.1.0-rc.1

This PR adds a transform an Embed to Paragraph option.
Here is an example.

Using an URL that automatically made an embed.
https://make.wordpress.org/core/2020/05/12/editor-chat-agenda-13-may-2020/

The embed:

Transform to Paragraph Block:

Transformed Embed block which has now turned into a Paragraph Block:

paaljoachim · 2020-05-12T14:14:45Z

Great job @desaiuditd !
Thank you very much for tackling this PR!
And a huge thank you to @mcsf !

@jorgefilipecosta
I do believe it would be beneficial to backport the embed PR to a WP 5.4.2 at a later time.
As it would create an important option for anyone in how they want a link to appear.

Add Block Transform to transform Embed blocks into Paragraph blocks.

f0d1f15

desaiuditd requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners September 12, 2019 03:33

desaiuditd commented Sep 12, 2019

View reviewed changes

Fix e2e tests.

708e1c4

desaiuditd requested review from gziolo, nerrad and ntwb as code owners September 12, 2019 06:03

desaiuditd added 2 commits September 12, 2019 18:06

Update block-transforms snapshot.

60a3ce4

Fix e2e tests.

69f6926

gziolo added [Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement. labels Nov 20, 2019

Merge branch 'master' into add/block-transform-to-transform-embed-blo…

2fa4e1b

…cks-into-paragraph-block # Conflicts: # packages/e2e-tests/specs/__snapshots__/block-transforms.test.js.snap

Merge branch 'master' into add/block-transform-to-transform-embed-blo…

bef557a

…cks-into-paragraph-block # Conflicts: # packages/block-library/src/embed/core-embeds.js # packages/block-library/src/embed/index.js # packages/e2e-tests/fixtures/block-transforms.js

This was referenced Apr 5, 2020

Option to use text link instead of auto embed. #15102

Closed

Bug: Link sometimes embeds other times not. #21029

Closed

mapk added the Needs Design Feedback Needs general design feedback. label Apr 14, 2020

Merge branch 'master' into add/block-transform-to-transform-embed-blo…

7a00726

…cks-into-paragraph-block

Add from transform for paragraph block and remove paste handler for e…

6d88232

…mbed block.

desaiuditd requested review from ellatrix and etoledom as code owners April 17, 2020 14:04

Revert changes to handle embeds smartly.

2b22121

paaljoachim mentioned this pull request Apr 23, 2020

Pasting links in Gutenberg #21789

Open

Merge branch 'master' into add/block-transform-to-transform-embed-blo…

2e838d9

…cks-into-paragraph-block

desaiuditd added 3 commits May 10, 2020 11:28

Merge branch 'master' into add/block-transform-to-transform-embed-blo…

1f54392

…cks-into-paragraph-block

Rever core embed transformations.

f8ecaa9

Remove DRY and keep the logic inline.

81d4f4c

desaiuditd requested a review from mcsf May 10, 2020 05:25

mcsf approved these changes May 11, 2020

View reviewed changes

mcsf merged commit 48aba1e into WordPress:master May 11, 2020

github-actions bot added this to the Gutenberg 8.1 milestone May 11, 2020

desaiuditd deleted the add/block-transform-to-transform-embed-blocks-into-paragraph-block branch May 11, 2020 13:14

ellatrix reviewed May 11, 2020

View reviewed changes

paaljoachim mentioned this pull request Sep 15, 2020

Embeds: WordPress Embeds not properly previewed in Editor #25344

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Block Transform to transform Embed blocks into Paragraph blocks. #17413

Add Block Transform to transform Embed blocks into Paragraph blocks. #17413

desaiuditd commented Sep 12, 2019

desaiuditd Sep 12, 2019 •

edited

Loading

desaiuditd Sep 12, 2019 •

edited

Loading

paaljoachim commented Apr 2, 2020 •

edited

Loading

desaiuditd commented Apr 5, 2020

paaljoachim commented Apr 5, 2020 •

edited

Loading

desaiuditd commented Apr 5, 2020

desaiuditd commented Apr 5, 2020 •

edited

Loading

desaiuditd commented Apr 17, 2020

desaiuditd commented Apr 18, 2020 •

edited

Loading

paaljoachim commented Apr 18, 2020

paaljoachim commented Apr 20, 2020

desaiuditd commented Apr 20, 2020

paaljoachim commented Apr 24, 2020 •

edited

Loading

mcsf commented Apr 27, 2020

desaiuditd commented May 10, 2020

mcsf left a comment

paaljoachim commented May 11, 2020 •

edited

Loading

mcsf commented May 11, 2020

paaljoachim commented May 11, 2020 •

edited

Loading

desaiuditd commented May 11, 2020

paaljoachim commented May 11, 2020

ellatrix May 11, 2020

mcsf May 11, 2020

jorgefilipecosta commented May 11, 2020

mcsf commented May 11, 2020

paaljoachim commented May 12, 2020 •

edited

Loading

paaljoachim commented May 12, 2020 •

edited

Loading

Add Block Transform to transform Embed blocks into Paragraph blocks. #17413

Add Block Transform to transform Embed blocks into Paragraph blocks. #17413

Conversation

desaiuditd commented Sep 12, 2019

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

desaiuditd Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

desaiuditd Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

paaljoachim commented Apr 2, 2020 • edited Loading

desaiuditd commented Apr 5, 2020

paaljoachim commented Apr 5, 2020 • edited Loading

desaiuditd commented Apr 5, 2020

desaiuditd commented Apr 5, 2020 • edited Loading

desaiuditd commented Apr 17, 2020

desaiuditd commented Apr 18, 2020 • edited Loading

paaljoachim commented Apr 18, 2020

paaljoachim commented Apr 20, 2020

desaiuditd commented Apr 20, 2020

paaljoachim commented Apr 24, 2020 • edited Loading

mcsf commented Apr 27, 2020

desaiuditd commented May 10, 2020

mcsf left a comment

Choose a reason for hiding this comment

paaljoachim commented May 11, 2020 • edited Loading

mcsf commented May 11, 2020

paaljoachim commented May 11, 2020 • edited Loading

desaiuditd commented May 11, 2020

paaljoachim commented May 11, 2020

ellatrix May 11, 2020

Choose a reason for hiding this comment

mcsf May 11, 2020

Choose a reason for hiding this comment

jorgefilipecosta commented May 11, 2020

mcsf commented May 11, 2020

paaljoachim commented May 12, 2020 • edited Loading

paaljoachim commented May 12, 2020 • edited Loading

desaiuditd Sep 12, 2019 •

edited

Loading

desaiuditd Sep 12, 2019 •

edited

Loading

paaljoachim commented Apr 2, 2020 •

edited

Loading

paaljoachim commented Apr 5, 2020 •

edited

Loading

desaiuditd commented Apr 5, 2020 •

edited

Loading

desaiuditd commented Apr 18, 2020 •

edited

Loading

paaljoachim commented Apr 24, 2020 •

edited

Loading

paaljoachim commented May 11, 2020 •

edited

Loading

paaljoachim commented May 11, 2020 •

edited

Loading

paaljoachim commented May 12, 2020 •

edited

Loading

paaljoachim commented May 12, 2020 •

edited

Loading