Embed URLs on paste from text block by ellatrix · Pull Request #2192 · WordPress/gutenberg

ellatrix · 2017-08-03T15:22:51Z

Still needs some polishing. Only YouTube works for now.

ellatrix · 2017-08-03T15:31:11Z

Asking for feedback for the general approach. We'll need to manually add matchers for every embed, not sure if we can pull this from core.

notnownikki

Would it be acceptable to call the oembed API with URLs and see if they return anything? That seems a bit heavy on the async calls, but thought I'd suggest it anyway :) does for maintain a list of regexs on the client side?

(Afk til Monday here, sorry for not looking up the client side of things in core myself!)

ellatrix · 2017-08-03T15:52:35Z

@notnownikki No worries! Yes this will have a client side list. I think server side will give us a strange experience. Then we have no idea what sort of URLs are embeddable, we need to wait until we hear back from the server, by which time the text can have changed. We could also set a general embed placeholder, but then need to take it away when it can't be embedded. That seems pretty strange.

With individual patterns we can make this experience a lot nicer than core atm. We can even show custom placeholders and block sizes if we want...

ellatrix · 2017-08-03T16:08:05Z

Ideally this list also has the block names, or some kind of identifier, and extra info we need on the client side such as title and icon. We also need to make sure the regular expressions are JS compatible

https://github.com/WordPress/WordPress/blob/4.8.1/wp-includes/class-oembed.php#L57

mtias · 2017-08-03T16:27:48Z

blocks/editable/patterns.js

 	} );

+	editor.on( 'pastepostprocess', () => {
+		setTimeout( space );


How does this affecting pasting non-url things?

That will be fine, but I agree it's better to separate. Still needs polishing :)

aduth · 2017-08-03T16:32:56Z

blocks/library/embed/index.js

 		},

+		transforms: {
+			from: matchers.map( ( regExp ) => ( {


While current direction of #1902 is to rename all references of "query" and "matchers" to "source", do you think we still ought to name this something more distinct? Like patterns ?

(parameter to RegExp constructor)

Patterns sounds great, didn't thing about the renaming, thanks!

aduth · 2017-08-03T16:44:46Z

blocks/library/embed/index.js

 	getEmbedBlockSettings( {
 		title: 'YouTube',
 		icon: 'video-alt3',
+		name: 'core-embed/youtube',


With some duplication here on block name, especially specific to the cases where we're providing patterns, do you have any thoughts on my suggestion at #2175 (review) for consolidating embed registration.

Maybe:

[ [ 'core/embed', 'Embed', 'video-alt3' ], [ 'core-embed/twitter', 'Twitter', 'twitter' ], [ 'core-embed/youtube', 'YouTube', 'video-alt3', [ /^\s*(https?:\/\/(?:m\.|www\.)?youtube\.com\/watch\S+)\s*/i, /^\s*(https?:\/\/(?:m\.|www\.)?youtube\.com\/playlist\S+)\s*/i, /^\s*(https?:\/\/youtu\.be\/\S+)\s*/i, ], ], // ... ].forEach( ( [ blockName, title, icon, patterns ] ) => { registerBlockType( blockName, getEmbedBlockSettings( { title, icon, blockName, patterns, } ) ); } );

(Or instead of array structure, an object { blockName, title, icon, patterns })

My main concern being the implied dependency between name and matchers. Since name is only being passed to getEmbedBlockSettings if matchers passed as well, this might mislead future maintainers to think they have access to name in future revisions to getEmbedBlockSettings. Or, otherwise, we pass name in all cases, but then we have excessive duplication.

Fine either way tbh. Still thinking maybe it makes more sense to pass attributes rather than a block, and create the block in the patterns plugin. That should always be the same anyway. I think I did the same for paste.

timmyc

This will be a great feature. I am a bit concerned about duplicating lists of supported embed providers between the core oembed class and the patterns file. Seems like a bit of overhead to maintain both, but understand the speed aspect of things.

I attempted to test using the following url: https://www.youtube.com/watch?v=ea2WoUtbzuw. The embed appears to begin taking place but then things go sideways and the following is shown in my console:

timmyc · 2017-08-04T00:09:03Z

blocks/library/embed/index.js

+		matchers: [
+			/^\s*(https?:\/\/(?:m\.|www\.)?youtube\.com\/watch\S+)\s*/i,
+			/^\s*(https?:\/\/(?:m\.|www\.)?youtube\.com\/playlist\S+)\s*/i,
+			/^\s*(https?:\/\/youtu\.be\/\S+)\s*/i,


As of r41179 core is now supporting YouTube embed URL patterns too, eg: https://www.youtube.com/embed/ea2WoUtbzuw

aduth · 2017-08-04T03:06:42Z

Should we create a Core API Task issue for listing all available embed patterns? Or bootstrap this data from PHP side†? Or just try running URL through oEmbed proxy endpoint and asynchronously updating if it returns a result? What about pasting custom embed providers? Scalability of this is concerning.

†

$oembed = _wp_oembed_get_object();
return $oembed->providers;

ellatrix · 2017-08-04T08:40:07Z

If we decide to also do OpenGraph, maybe this is not needed... #2047 (comment)

Then we just embed every URL that is pasted on its own line.

codecov · 2017-08-04T13:42:07Z

Codecov Report

Merging #2192 into master will increase coverage by 0.28%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2192      +/-   ##
==========================================
+ Coverage   22.99%   23.28%   +0.28%     
==========================================
  Files         141      141              
  Lines        4393     4420      +27     
  Branches      746      748       +2     
==========================================
+ Hits         1010     1029      +19     
- Misses       2852     2859       +7     
- Partials      531      532       +1

Impacted Files	Coverage Δ
blocks/library/separator/index.js	`50% <ø> (ø)`	⬆️
blocks/library/code/index.js	`50% <ø> (ø)`	⬆️
blocks/editable/patterns.js	`1.81% <0%> (ø)`	⬆️
blocks/library/embed/index.js	`45.45% <0%> (-0.53%)`	⬇️
editor/enable-tracking-prompt/index.js	`95.23% <0%> (-4.77%)`	⬇️
editor/index.js	`0% <0%> (ø)`	⬆️
blocks/library/table/index.js	`41.17% <0%> (+4.81%)`	⬆️
editor/actions.js	`44.11% <0%> (+4.83%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d95faa5...f8e6208. Read the comment docs.

jasmussen · 2017-08-04T15:15:09Z

I love this.

I think it might be nice to require you to press enter before the link is converted. I'm not sure. Perhaps it's okay to get this in as is, to get feedback, and then address as needed?

aduth · 2017-08-04T15:26:47Z

blocks/library/embed/index.js

+				{
+					type: 'pattern',
+					trigger: 'paste',
+					regExp: /^\s*(https:\/\/\S+)\s*/i,


The s in https should be optional: https?

I think all URLs will be https, but sure, let's be more tolerant.

aduth

We ought to handle the case where the pasted URL is not embeddable. Currently it converts to an embed block, even when the oEmbed proxy returns with code: "oembed_invalid_url"

aduth · 2017-08-04T16:01:47Z

blocks/library/code/index.js

 		from: [
 			{
 				type: 'pattern',
+				trigger: 'enter',


Aside: These patterns are hard to discover. Didn't realize I had to Shift+Enter to trigger the transform.

They're broken in master. I need to fix it.

(Should work en plain enter.)

ellatrix · 2017-08-04T17:46:43Z

@aduth Are you okay with iterating on this?

mtias · 2017-08-04T18:13:59Z

🚢

aduth · 2018-01-15T19:12:40Z

blocks/editable/patterns.js

 	} );

+	editor.on( 'pastepostprocess', () => {
+		setTimeout( () => searchFirstText( pastePatterns ) );


With the setTimeout here (and the more recent inline handling), we need to make sure that the editor still exists in searchFirstText, since because it is called asynchronously, the editor might be removed in the time since scheduled. I discovered this in my own work (caused by an unrelated bug), but was difficult to track down the cause of the error'ing that occurs here.

Yeah, I noticed some issues too in another PR. While this specific line has been removed, it's still an issue in the keydown event callback.

ellatrix force-pushed the try/embed-pattern branch from 57368ed to 9ebd63a Compare August 3, 2017 15:24

ellatrix requested review from mtias and notnownikki August 3, 2017 15:30

notnownikki reviewed Aug 3, 2017

View reviewed changes

ellatrix force-pushed the try/embed-pattern branch 2 times, most recently from 5e4e1a0 to 646efe9 Compare August 3, 2017 16:04

mtias reviewed Aug 3, 2017

View reviewed changes

aduth reviewed Aug 3, 2017

View reviewed changes

timmyc reviewed Aug 4, 2017

View reviewed changes

ellatrix force-pushed the try/embed-pattern branch 2 times, most recently from 180e7b7 to d38ff02 Compare August 4, 2017 13:42

ellatrix added 3 commits August 4, 2017 17:03

Embed URLs on paste from text block

bafd01d

Attempt to embed all URLs

29a84f3

Split enter and space patterns

aec2017

ellatrix force-pushed the try/embed-pattern branch from 55a1e8b to aec2017 Compare August 4, 2017 15:06

aduth requested changes Aug 4, 2017

View reviewed changes

Allow non-https URLs

f8e6208

aduth requested changes Aug 4, 2017

View reviewed changes

ellatrix merged commit d80dd50 into master Aug 4, 2017

ellatrix deleted the try/embed-pattern branch August 4, 2017 18:14

mtias mentioned this pull request Aug 7, 2017

Text block: Oembed URLs directly #1033

Closed

aduth reviewed Jan 15, 2018

View reviewed changes

ellatrix mentioned this pull request Jan 16, 2018

Ensure editor still exists after timeout #4524

Merged

Conversation

ellatrix commented Aug 3, 2017

Uh oh!

ellatrix commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notnownikki left a comment

Choose a reason for hiding this comment

Uh oh!

ellatrix commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellatrix commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

timmyc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Aug 4, 2017

Uh oh!

ellatrix commented Aug 4, 2017

Uh oh!

codecov bot commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jasmussen commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

ellatrix commented Aug 4, 2017

Uh oh!

mtias commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ellatrix commented Aug 3, 2017 •

edited

Loading

ellatrix commented Aug 3, 2017 •

edited

Loading

ellatrix commented Aug 3, 2017 •

edited

Loading

aduth Aug 3, 2017 •

edited

Loading

codecov bot commented Aug 4, 2017 •

edited

Loading