-
Notifications
You must be signed in to change notification settings - Fork 385
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
Conversation
Fix amp-fit-text selectores.
Hi @miina, 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. |
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. |
Looks like At least now it works locally, hopefully, fixes it here, too. |
@swissspidy Mind giving it another review? |
placeholder={ placeholder || __( 'Write text…', 'amp' ) } | ||
/> | ||
{ isEditing && | ||
<RichText |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Not sure why the CTA warning message test is failing here, not happening locally. Looking into this ... |
Looks like the tests are OK now finally. |
Closes #2783.
Requires double click for editing the Text block.
Todo: