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

Image Editor: Add Inline Edit Button #14143

Merged
merged 5 commits into from
May 18, 2017
Merged

Conversation

msmithgu
Copy link
Contributor

Regarding issue #8306 .

Clicking on an image in the post editor brings up an in-context menu:
screen shot 2017-04-27 at 3 44 28 pm

This PR adds an edit icon (the pencil) to the in-context menu:
screen shot 2017-04-27 at 3 34 42 pm

When we click on that the inline edit button, it brings up the gallery with the clicked on image pre-selected. I found when testing out usability that being able to swap the image with others was desirable and less confusing.

This completes #8306 .

To Test

  1. Edit a post with an image.
  2. Click on the image to see the in-context menu.
  3. Click on the pencil icon to see the new behavior.

@msmithgu msmithgu self-assigned this May 16, 2017
@matticbot matticbot added OSS Citizen [Size] L Large sized issue labels May 16, 2017
@designsimply designsimply added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 16, 2017
@msmithgu msmithgu added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 16, 2017
@msmithgu msmithgu requested review from gwwar, ellatrix and dmsnell May 16, 2017 23:20
@msmithgu msmithgu added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 16, 2017
@msmithgu msmithgu requested a review from designsimply May 16, 2017 23:24
Copy link
Member

@jblz jblz left a comment

Choose a reason for hiding this comment

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

Context -- I started working on this independently from you in #14086 before I realized your PR existed, sorry I missed your comment!

@@ -68,6 +68,7 @@ function wpEditImage( editor ) {
'wp_img_alignnone',
'wpcom_img_size_decrease',
'wpcom_img_size_increase',
'wp_img_edit', // See plugins/media
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this down 2 lines so the button appears next to the remove / delete button

editor.addButton( 'wp_img_edit', {
tooltip: i18n.translate( 'Edit', { context: 'verb' } ),
icon: 'dashicon dashicons-edit',
onclick: function() {
Copy link
Member

Choose a reason for hiding this comment

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

I'd add in a line:
classes: 'toolbar-segment-start',

...such that the Edit button is "grouped" with the next button (as I mention in the other inline comment, I feel it's conceptually similar to Delete).

@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels May 17, 2017
@msmithgu
Copy link
Contributor Author

@jblz Got your changes in.

@gwwar I fixed the issue crop duplicate issue I found yesterday.

I think it's ready now. Need more eyes on it. At XL size now, I want to get this out.

@gwwar
Copy link
Contributor

gwwar commented May 17, 2017

@msmithgu do you mind rebasing with master? There appear to be many unrelated commits in this branch.

@msmithgu msmithgu force-pushed the add/inline-image-editing branch from 5e889c8 to bf9a929 Compare May 17, 2017 16:03
@matticbot matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels May 17, 2017
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.

We're very close, I retested edit/edit vs edit/delete behavior.

I did notice an extra case, when the edited image is still uploading to the server, which creates a duplicate image in the dom.

To Reproduce:

  • Insert an image
  • Select it, click the edit pencil on the toolbar
  • Click Edit, then edit the photo
  • Make some changes in the image editor
  • Save the image
  • Before the image finishes updating, update the new value. (It may help to throttle your internet connection in dev tools).

edit


// If we were pasting into an img, remove it so it's replaced
// with the new one.
if ( node.nodeName === 'IMG' || node.nodeName === 'DT' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that this wraps the node with a <DT /> after editing an image.

@gwwar
Copy link
Contributor

gwwar commented May 17, 2017

Whoops, I should have re-read #307 sooner. It does seem like jumping directly into the details view vs the list view would be much more helpful, especially when the user has a lot of media items.

cc @msmithgu @drw158

visible: true,
labels: {
confirm: i18n.translate( 'Update', { context: 'verb' } )
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We can set the view with an options object:

renderModal(
	{
		visible: true,
		labels: {
			confirm: i18n.translate( 'Update', { context: 'verb' } )
		}
	},
	{
		view: ModalViews.DETAIL
	}
);

@gwwar
Copy link
Contributor

gwwar commented May 18, 2017

@msmithgu before merging I would recommend updating behavior to open in the detail view. We can probably open up another issue for the dup case for images that are still loading.

@gwwar
Copy link
Contributor

gwwar commented May 18, 2017

I added #14233 so we can tackle that next

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.

Let's :shipit:

@gwwar gwwar added [Status] Ready to Merge and removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 18, 2017
@msmithgu msmithgu merged commit dba0a05 into master May 18, 2017
@msmithgu msmithgu deleted the add/inline-image-editing branch May 18, 2017 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. OSS Citizen [Size] L Large sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants