Editor: Preview embed when editing URL #18171
Conversation
iandunn
added
[Feature] Post/Page Editor
iandunn
force-pushed
the
add/preview-embed-dialog
branch
3 times, most recently
from
56a2085
to
d8d8040
Compare
|
@bisko, if you have time to take a look, the main thing I'm stuck on is https://github.com/Automattic/wp-calypso/pull/18171/files#diff-84faf6cda9cb8b4bb7b5f7eec3bfb2c1R62 Thanks! |
iandunn
force-pushed
the
add/preview-embed-dialog
branch
from
d8d8040
to
55c165c
Compare
iandunn
force-pushed
the
add/preview-embed-dialog
branch
from
55c165c
to
eae4474
Compare
iandunn
force-pushed
the
add/preview-embed-dialog
branch
from
eae4474
to
a4e0576
Compare
|
Thanks for taking this over @bisko :) I cleaned things up a bit and force-pushed my local branch. I didn't squash the "direct api call" commit yet, just in case it's useful to see what the approach was before that. I was planning to squash it before merging, though. Other than that, I think it's ready to hand off. Let me know if you have any questions or any of my notes aren't clear, though. Thanks again! |
|
Oh, also, don't worry about making the |
bisko
force-pushed
the
add/preview-embed-dialog
branch
3 times, most recently
from
7352b27
to
b6bcc5e
Compare
bisko
force-pushed
the
add/preview-embed-dialog
branch
from
f8b9dcc
to
d8e7bf9
Compare
bisko
added
[Status] Needs Review
There was a problem hiding this comment.
Awesome! This is working pretty well.
A couple of things:
Can you please rebase so we can make sure the interaction with the editor redesign is solid?
Also, I'm noticing a bug w/ youtube & cache hits.
To repro:
- embed a youtube URL
- Edit the video id argument to a non-existing one (deleting a letter usually works)
- Wait for the load... it shows a "blank" placeholder youtube video (which seems like a bug as well)
- Restore the original video id argument
I expected the embedded video to be corrected, but the placeholder remains.
Screencap: https://cloudup.com/c1LfECyt_DH
| /** | ||
| * Prepare the markup for the new embed URL | ||
| * | ||
| * @todo: Making an API request directly is a bit of a hack. The ideal solution would be to |
There was a problem hiding this comment.
I like the description of the tradeoffs made here and rationale. Thanks for including it!
There was a problem hiding this comment.
cc @iandunn ^ 🙇 Thanks for adding that!
There was a problem hiding this comment.
Thanks! I'm glad that was helpful :)
…improve loading/fetching data from the backend
…t to make it more extensible
bisko
force-pushed
the
add/preview-embed-dialog
branch
from
2f4c747
to
cb581d2
Compare
|
@jblz PR rebased and fixed the issue you mentioned. Now it should properly load cached results. |
There was a problem hiding this comment.
This is working really well. Tested w/ various youtube, instagram, etc. embeds and all work well with the preview.
Let's go ahead and merge this and come back w/ follow-ups to:
- disconnect the example component from the state tree
- add an entry to the "Add" Editor feature
| connectedEmbedDialogExample.displayName = 'EmbedDialogExample'; | ||
|
|
||
| // todo | ||
| // connecting this component feels wrong. it's an example, so shouldn't it instantiate EmbedDialog with renderWithReduxStore |
There was a problem hiding this comment.
I tend to agree here. Are we just providing the siteId here because it's required by some child component or something? I don't think this is a blocker for this PR so we can get this launched, but I think we should do what it takes to disconnect this example from the state tree in a follow-up.
jblz
added
[Status] Ready to Merge
and removed
[Status] Needs Review
|
🎉 Kudos for seeing this through to the end :) |
This is an iteration on #17152. It adds a preview of the new embed, in order to fully implement the design in #1729.
Before
After
See #1729