Editor: Add support for editing embeds inside a post #17152

iandunn · 2017-08-12T20:13:31Z

This is still a work in progress.

Currently embeds can't be edited after they've been inserted into a post. They can only be deleted and then re-created with the new parameters.

This PR adds a method for editing them. It doesn't add a preview of the new embed, but that will be done in #18171.

See #1729

Test plan:

Click on an embed, click the pencil icon to open the dialog, change the URL, click cancel button. Dialog should close, the embed should not be updated. Open dialog again, the original URL should be shown.
Open the embed and change the URL, click update. The embed should be updated to the new URL.
Test that esc and clicking outside the box will close the modal
Test that enter in the text field will submit the new URL and update the embed
The input field should be automatically focused when the dialog opens
Add two embeds to page. Open one, cancel it, then open the other. The correct URLs should be shown for each embed (rather than showing the URL for the first embed when you edit the second embed).

matticbot · 2017-08-12T20:13:37Z

Test live: https://calypso.live/?branch=add/edit-embed-dialog

iandunn · 2017-09-30T00:43:53Z

This is ready for review now. Here's an example of what it should look like:

iandunn · 2017-09-30T00:54:13Z

The CI failure is just the linter complaining about an unrelated violated that exists in master. I cleaned up some of the simpler violations in 5bb75e5, but didn't want to go down the rabbit hole with the more complex ones right now, since this PR has already dragged on too long :)

spen

Looks good on my first pass - I've mentioned just a few little nits for now but happy give a proper run through & review over the weekend :)

spen · 2017-09-30T01:04:37Z

client/components/tinymce/plugins/embed/dialog.jsx

+import Button from 'components/button';
+import Dialog from 'components/dialog';
+import FormTextInput from 'components/forms/form-text-input';
+import { localize } from 'i18n-calypso';


Nit: We usually pile 'i18n-calypso' in to externals since it's source is outside of Calypso (even though really it's calypso specific)

spen · 2017-09-30T01:16:39Z

client/components/tinymce/plugins/embed/dialog.jsx

+		} );
+	};
+
+	onUpdate = () => {


It may be worth debouncing both of the input events for perf reasons.

Did you mean that onChangeEmbedUrl (the <input onChange... handler) should be debounced, or onUpdate and onCancel? The latter two only fire once when the Update and Cancel buttons are clicked, unless I'm missing something.

I'm planning on debouncing part of onChangeEmbedUrl in #18171 , because that PR will add a preview embed of the new URL:

https://github.com/Automattic/wp-calypso/pull/18171/files#diff-e88450610ed12ecb3fc80bffa4181e72R65

It doesn't debounce the setState() call, though. I would think that would cause problems, since it's a controlled component. Wouldn't it interfere with the user typing a new URL in quickly, or am I misunderstanding how it works? It seems to work ok in my tests, and I don't see any warnings about it when searching, but I just wanna double check.

Oops, yes sorry, onChangeEmbedUrl :P I'm glad you know which I meant!
I feel like debouncing all of onChangeEmbedUrl might be sensible to avoid re-renders on every keypress (since the state is changing).

It would also avoid setting focus on every key up in #18171 - not sure if that helps or hinders that situation though.

👍 thanks, I'll go with that approach :)

Hmmm, I added the debounce in 2134a11, but that broke the unit tests and introduced a bug where clicking Update before the debounce callback has ran will cause the old URL to be returned to TinyMCE instead of the new one.

Those are fixable problems, but I'm just wondering if it's worth making the code more complex to avoid calling render() here, especially since I don't think there'll be any diffs between the virtual DOM and the real one, since FormTextInput is an uncontrolled component. Also because I think the most common use case will be copy/pasting a URL, rather than manually typing it.

I think previewing the new URL in #18171 should definitely be debounced, but I'm just not sure that setState should be.

facebook/react#1360 (comment)

Is there a good way to tell what kind of performance impact setState will have if left undebounced? Maybe profiling w/ Chrome dev tools or something? I can look into that tomorrow if you think it's worth it.

spen · 2017-09-30T01:20:17Z

client/components/tinymce/plugins/embed/dialog.jsx

+
+	render() {
+		const { translate } = this.props,
+			dialogButtons = [


I'm not sure it's in the guidelines but the trend is to define each const or let separately - I believe for the same reasons mentioned in AirBnBs guidelines.

Ah, nice! I often prefer that as well. It doesn't look like there's anything about it in the docs, I think I was just doing it out of habit from the Core guidelines.

spen · 2017-09-30T01:21:43Z

client/components/tinymce/plugins/embed/dialog.jsx

+				</h3>
+
+				<FormTextInput
+					autoFocus={ true }


little tip: ={ true } is redundant here as a bare prop (autoFocus without assignment) would infer the value true... totally fine to leave as is though!

Ah, good to know, thanks! I tested both out, and having the explicit true feels a bit nicer to me in this specific case, but it's definitely not a big deal. I can also see how leaving it off could come in handy in some cases, like <Foo autoFocus foo bar={ false }>.

I definitely see the upside to it being explicit - it doesn't require knowing that a bare prop implies a true value for a start :)

I think in this case the main thing that makes it feel "better" to me is just the visual consistency with the other parameters. No big deal either way, though :)

spen · 2017-09-30T01:24:18Z

client/components/tinymce/plugins/embed/docs/example.jsx

+					isVisible={ this.state.showDialog }
+					onCancel={ this.onCancel }
+					onUpdate={ this.onUpdate }
+					translate={ identity }


This might be a little misleading to mention in the example. I'm not sure what others think but I'd leave it out since the reader of the example shouldn't need to consider it.

Are you referring to translate? I added it here since it's a required prop.

#18064 (comment)

Ahh, I'm with you - I didn't spot that the base component was being imported directly :)
That said, should we be recommending it's use in the example or should we recommend using the enhanced component ( export default )?
I could be wrong but I think the only reason one would need to import the base component would be for test purposes.

Ah, I get what you're saying now, and you're totally right. The example should use the default, localized component, since that's what would normally be used when adding new uses of the component elsewhere in Calypso. Great catch, thanks!

iandunn · 2017-10-02T20:25:42Z

I pushed some of those tweaks in 6ba0690.

spen · 2017-10-02T21:40:06Z

client/components/tinymce/plugins/embed/dialog.jsx

+		} );
+	};
+
+	onUpdate = () => {


Oops, yes sorry, onChangeEmbedUrl :P I'm glad you know which I meant!
I feel like debouncing all of onChangeEmbedUrl might be sensible to avoid re-renders on every keypress (since the state is changing).

It would also avoid setting focus on every key up in #18171 - not sure if that helps or hinders that situation though.

spen · 2017-10-02T21:47:04Z

client/components/tinymce/plugins/embed/docs/example.jsx

+					isVisible={ this.state.showDialog }
+					onCancel={ this.onCancel }
+					onUpdate={ this.onUpdate }
+					translate={ identity }


Ahh, I'm with you - I didn't spot that the base component was being imported directly :)
That said, should we be recommending it's use in the example or should we recommend using the enhanced component ( export default )?
I could be wrong but I think the only reason one would need to import the base component would be for test purposes.

spen · 2017-10-02T21:52:12Z

client/components/tinymce/plugins/embed/test/dialog.js

+/**
+ * External dependencies
+ */
+import Dialog from 'components/dialog';


Though I get that they are external to this 'unit' I think we should keep import Dialog from 'components/dialog'; and import FormTextInput from 'components/forms/form-text-input'; in the internal deps.

Doh, I don't think I meant to put those in External; thanks for catching that :)

marekhrabe · 2017-10-04T07:10:00Z

I didn’t dig very deep into the PR, but a general rule is you don’t want any delay with onChange. React relies on the fact that after pressing a key, it will receive the new value property in the very next render, otherwise, the user input would be canceled and value set back to the previous one. User might experience disappearing letters as they type or other unexpected behavior

I understand you need to debounce the iframe updates. I would definitely do that. Simplest way would be to have a second state like previewUrl and debounce its assignment, based on the original state

spen · 2017-10-04T10:03:32Z

@marekhrabe's completely right! Sorry Ian, I think I'd just got myself confused and passed that confusion on here!

spen · 2017-10-04T10:13:40Z

For clarity: I think you were on the right path in the first place - Updating the URL as the user types but debouncing the fetching.

iandunn · 2017-10-05T01:13:07Z

Thanks for the input :)

I've removed the debounce and rebased. Any final thoughts before I merge?

iandunn · 2017-10-05T01:20:07Z

( CI failure is just an unrelated linter warning )

spen · 2017-10-05T10:23:50Z

This is definitely outside the scope of this PR and something I don't think we can really get around, but editing the url of 1 (of 2) embedded youtube videos causes both of them to 'flash' (I'm guessing it's some effect of re-rendering.
Just making note of it here before I forget to mention elsewhere :)

Other than that, this is looking great! Thanks Ian and sorry again for the curveball! 😃

This adds a dialog to edit the embed URL, but doesn't add a preview of the new URL, so it doesn't fully implement [the design in #1729](https://user-images.githubusercontent.com/191598/28643536-23f2e318-7224-11e7-8fd1-9a889d53b594.png). It's a step in that direction, though, and a future PR will add the preview. See #1729

iandunn · 2017-10-05T15:16:56Z

Thanks! Merged :)

iandunn added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] In Progress labels Aug 12, 2017

matticbot added the [Size] L Large sized issue label Aug 12, 2017

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Aug 18, 2017

iandunn force-pushed the add/edit-embed-dialog branch 2 times, most recently from 0fb745a to f680276 Compare August 24, 2017 00:58

iandunn self-assigned this Sep 19, 2017

iandunn force-pushed the add/edit-embed-dialog branch 2 times, most recently from 9c99a93 to 8084d6b Compare September 21, 2017 23:26

iandunn mentioned this pull request Sep 22, 2017

Editor: Preview embed when editing URL #18171

Merged

iandunn force-pushed the add/edit-embed-dialog branch 3 times, most recently from b38dc12 to fab919e Compare September 30, 2017 00:42

iandunn added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Sep 30, 2017

spen reviewed Sep 30, 2017

View reviewed changes

iandunn force-pushed the add/edit-embed-dialog branch from fab919e to 6ba0690 Compare October 2, 2017 20:06

spen reviewed Oct 2, 2017

View reviewed changes

iandunn force-pushed the add/edit-embed-dialog branch from 2134a11 to d2ba91c Compare October 5, 2017 01:11

spen approved these changes Oct 5, 2017

View reviewed changes

iandunn added 2 commits October 5, 2017 07:53

Editor: Correct style guide deviations in plugin.js

cb5eae9

iandunn force-pushed the add/edit-embed-dialog branch from d2ba91c to 92bb1d2 Compare October 5, 2017 15:00

iandunn merged commit ee273bb into master Oct 5, 2017

iandunn deleted the add/edit-embed-dialog branch October 5, 2017 15:04

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 5, 2017

iandunn mentioned this pull request Oct 5, 2017

Editor: add embed editing #1729

Closed

iandunn added the [Tests] Includes Tests label Oct 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Add support for editing embeds inside a post #17152

Editor: Add support for editing embeds inside a post #17152

iandunn commented Aug 12, 2017 •

edited

Loading

matticbot commented Aug 12, 2017

iandunn commented Sep 30, 2017

iandunn commented Sep 30, 2017

spen left a comment

spen Sep 30, 2017

spen Sep 30, 2017

iandunn Oct 2, 2017

spen Oct 2, 2017

iandunn Oct 3, 2017

iandunn Oct 4, 2017

spen Sep 30, 2017

iandunn Oct 2, 2017

spen Sep 30, 2017

iandunn Oct 2, 2017

spen Oct 2, 2017

iandunn Oct 3, 2017

spen Sep 30, 2017

iandunn Oct 2, 2017

spen Oct 2, 2017

iandunn Oct 3, 2017

iandunn commented Oct 2, 2017

spen Oct 2, 2017

spen Oct 2, 2017

spen Oct 2, 2017

iandunn Oct 3, 2017

spen Oct 3, 2017

marekhrabe commented Oct 4, 2017

spen commented Oct 4, 2017

spen commented Oct 4, 2017

iandunn commented Oct 5, 2017

iandunn commented Oct 5, 2017

spen commented Oct 5, 2017 •

edited

Loading

iandunn commented Oct 5, 2017

Editor: Add support for editing embeds inside a post #17152

Editor: Add support for editing embeds inside a post #17152

Conversation

iandunn commented Aug 12, 2017 • edited Loading

Test plan:

matticbot commented Aug 12, 2017

iandunn commented Sep 30, 2017

iandunn commented Sep 30, 2017

spen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iandunn commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekhrabe commented Oct 4, 2017

spen commented Oct 4, 2017

spen commented Oct 4, 2017

iandunn commented Oct 5, 2017

iandunn commented Oct 5, 2017

spen commented Oct 5, 2017 • edited Loading

iandunn commented Oct 5, 2017

iandunn commented Aug 12, 2017 •

edited

Loading

spen commented Oct 5, 2017 •

edited

Loading