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

Require double click (or just two clicks) for editing. #2863

Merged
merged 52 commits into from
Aug 16, 2019

Conversation

miina
Copy link
Contributor

@miina miina commented Jul 22, 2019

Closes #2783.

Requires double click for editing the Text block.

Todo:

  • Display edit mode only on double click.
  • Remove dragging area border and related logic.
  • Don't allow dragging in edit mode.
  • Better focus when going into edit mode.
  • e2e tests.

@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 22, 2019
@kienstra
Copy link
Contributor

kienstra commented Jul 25, 2019

Hi @miina,
This looks good. Editing and dragging of the Text block works well:

dragging-editing

In my opinion, requiring a double-click to edit might be a little too much.

For example, Preview from macOS also allows dragging the entire text element. But editing text only requires one click, once the text is focused.

Though that's a requirement from #2783, and this PR applies it well.

editing-here

@swissspidy
Copy link
Collaborator

In my opinion, requiring a double-click to edit might be a little too much.

Agreed. When the block is already selected, just requiring a single click would be nice. Not sure if that causes conflicts with dragging though.

@miina
Copy link
Contributor Author

miina commented Jul 25, 2019

Agreed. When the block is already selected, just requiring a single click would be nice. Not sure if that causes conflicts with dragging though.

Will try it out.

@swissspidy
Copy link
Collaborator

Just merged #2824, so this needs some updates.

That PR mentions:

Update the logic once #2863 gets merged.

@miina
Copy link
Contributor Author

miina commented Aug 13, 2019

Looks like core-editor.test.js was failing due to switchEditorModeTo not waiting for the selector being available.

At least now it works locally, hopefully, fixes it here, too.

@miina
Copy link
Contributor Author

miina commented Aug 13, 2019

@swissspidy Mind giving it another review?

@miina miina requested a review from kienstra August 13, 2019 20:51
placeholder={ placeholder || __( 'Write text…', 'amp' ) }
/>
{ isEditing &&
<RichText
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it would make sense to move this to DraggableText as well actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could rename it to DraggableRichText. Would make a more complete component. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although maybe it would make the component confusing with that many attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for now we can keep it as-is.

@miina
Copy link
Contributor Author

miina commented Aug 14, 2019

Not sure why the CTA warning message test is failing here, not happening locally. Looking into this ...

@miina
Copy link
Contributor Author

miina commented Aug 14, 2019

Looks like the tests are OK now finally.

@miina miina mentioned this pull request Aug 16, 2019
4 tasks
@miina miina merged commit 3362b66 into develop Aug 16, 2019
@miina miina deleted the add/2783-double_click_edit branch August 16, 2019 14:21
@westonruter westonruter added this to the v1.2.1 milestone Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
5 participants