Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain image selection in post after updateMedia #14328

Merged
merged 4 commits into from
May 22, 2017

Conversation

msmithgu
Copy link
Contributor

This addresses #14233

When an edited image didn't have a caption and was updated, it created a duplicate image in the post. This was because updateMedia would sometimes lose the current selection. This change fixes this issue.

@msmithgu msmithgu self-assigned this May 22, 2017
@msmithgu msmithgu requested review from gwwar, jblz and dmsnell May 22, 2017 15:15
@matticbot matticbot added OSS Citizen [Size] S Small sized issue labels May 22, 2017
@@ -391,6 +392,21 @@ function mediaButton( editor ) {
// edited and marked dirty, we can set the counter to correct number.
numOfImagesToUpdate = null;
}

// Re-select image node if one was selected.
Copy link
Member

Choose a reason for hiding this comment

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

The cause of the problem really isn't clear and this comment doesn't seem to explain it much either. Could we leave more context to indicate why this code exists?

@matticbot matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels May 22, 2017
@gwwar gwwar added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 22, 2017
@@ -393,6 +400,19 @@ function mediaButton( editor ) {
}
} );

function reselectImage() {
// Re-select image node
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this comment since the function is named aptly. If this needs further explication we can add jsDoc to the function.

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Thanks @msmithgu this tests well for me

@msmithgu msmithgu merged commit 354ec14 into master May 22, 2017
@msmithgu msmithgu deleted the fix/duplicate-image-in-post branch May 22, 2017 17:06
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 22, 2017
@@ -377,6 +378,12 @@ function mediaButton( editor ) {
editor.fire( 'SetTextAreaContent', { content } );
}

// We have just replaced some image nodes, potentially losing selection/focus.
// So if an image node was selected and one isn't now, re-select it.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the comment update. I think it gets us closer but not fully to our ideal. if I read this I would have no idea that a duplication bug existed without it.

is duplicating the image directly related to losing selection? what would happen if we didn't reselect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Testing steps are available in #14233

cc @msmithgu it's generally nice to copy over testing instructions to the related PR. This lets folks avoid having to context switch to another page and potentially avoid reading a long conversation on what needs to be done about an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.. there is other code that replaces or appends depending on whether an image is selected. That code is intended to match usability behavior of core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The duplication bug occurred because an edited image was getting deselected, then modified, and thusly appended rather than replaced.

This change fixes the cause of that issue: the deselection.

Copy link
Member

Choose a reason for hiding this comment

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

an edited image was getting deselected, then modified, and thusly appended rather than replaced.

that sounds like the basis for a great comment in the code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Citizen [Size] M Medium sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants