Writing flow: Copy whole block if no text is selected #22186
Conversation
mcsf
added
the
[Feature] Writing Flow
|
Size Change: +329 B (0%) Total Size: 832 kB
ℹ️ View Unchanged
|
|
Sounds good. The only thing I would find a bit strange is that there's no visual indication of what you're copying. If you select some text, it's visible, and clear what you copy. If you select some blocks, you see the selection and what you copy. That's not the case there. On the other hand the shortcut doesn't do anything, so it doesn't really hurt to add it. :) |
|
Do you think an e2e test could be useful? The shortcuts work in content editable fields in our e2e tests. |
|
Great! There are a few things that I think could be improved:
|
|
Yeah, I added it to the block actions. We should add the shortcut there afterwards too, but that will require a follow up PR with some ClipboardButton refactoring. |
|
@ellatrix and @mtias, thanks for the reviews.
I do, but haven't taken the time to write any yet.
Done in 99cf442. I'm not yet sure of the implementation, but how's the UX?
Done too, and expanded to cutting actions.
Yes. So far it feels alright to me. |
| toggleBlockHighlight( clientId, true ); | ||
| const timeout = setTimeout( () => { | ||
| toggleBlockHighlight( clientId, false ); | ||
| }, 1000 ); | ||
| timeouts.current.push( timeout ); |
There was a problem hiding this comment.
This feels a lot like a hack. Does it have its place here? Should the idea of flashing be a proper block-editor action? Should there be different primitives for this? cc @youknowriad
There was a problem hiding this comment.
The dispatch('core/editor').flashBlock( clientId ) action can also be written as a generator and absorbs the timeout handling to unset the reducer state. When I call dispatch('core/editor').flashBlock( clientId ) I should expect it to handle all the logic.
There was a problem hiding this comment.
Thanks, I agree.
One last question: did you namespace it core/editor intentionally, as opposed to core/block-editor?
There was a problem hiding this comment.
no, not intentional :) good catch.
There was a problem hiding this comment.
I pushed f425709, but I don't really like what I did with the introduction of a SLEEP control. Do you suggest a different approach, @youknowriad? And if not, should I export sleep from controls, or keep it private as now?
There was a problem hiding this comment.
I think the "sleep" control is totally fine. redux-saga, for instance, has a "delay" effect which is equivalent.
Made it a bit faster in f0a8693 |
mcsf
force-pushed
the
add/copy-whole-block-on-ctrl-c
branch
from
f425709
to
707b606
Compare
mcsf
force-pushed
the
add/copy-whole-block-on-ctrl-c
branch
from
9ff0834
to
f305234
Compare
|
I think this is good now with the addition of tests. Thanks for everyone's help! |
| <!-- wp:paragraph --> | ||
| <p></p> | ||
| <!-- /wp:paragraph -->" |
There was a problem hiding this comment.
For the record, I don't like these empty paragraphs that the pasting yields, but this is tangent to the PR itself.
|
I'm seeing a potential bug with this PR on version 8.2.0 as well as 8.2.1 In Embed block, copying the embed url after selecting it in the placeholder is copying the whole block instead of just url. Please refer to the screencast.
@mcsf Could this be a regression? |
|
Good catch! That definitely sounds related. Could you file an issue and let me know? Thanks! |
|
Hi, I'm getting the same problem when trying to copy or cut individual words/lines in Custom HTML blocks. The whole block is copied instead of just the selected text. |
On the other hand, I was thinking, can this feature be opt in? Copying text even within custom blocks, inner blocks and other areas is trivial and important user action. So if we make it opt-in, we can avoid such bugs from creeping into users’ daily workflow. Power users can opt-in for this if they want to. And that way we can limit the scope of such bugs. |
|
Fix in #22793 up for review.
Bugs are always frustrating, and unfortunately they will creep up sooner or later. That shouldn't however be a reason for us to fragment the user experience by selectively disabling core features. It should instead fuel our efforts to ensure quality and stability in the project. In this case, I might have tested the feature more broadly, I might have added more varied E2E tests. In short, I think it would be a great mistake to make this opt-in. |
Description
That is, when a single block is selected but no text is selected, copying (such as using CTRL-C) and cutting (CTRL-X) now grab the entire block. This is an extension of the behaviour that was in place for selections of multiple blocks.
Subsequently added:
Questions
I extended this to cut (CTRL-X) to see how it feels. Does it make sense?
How has this been tested?
Screenshots
Types of changes
Checklist: