List View: Add keyboard clipboard events for cut, copy, paste #57838
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Enhancement
|
Size Change: -3.52 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewserong
force-pushed
the
try/add-clipboard-events-to-the-list-view
branch
from
9924e91
to
8edc4c6
Compare
andrewserong
changed the title
andrewserong
requested review from
kevin940726,
Mamaduka and
ellatrix
as code owners
andrewserong
force-pushed
the
try/add-clipboard-events-to-the-list-view
branch
from
674638d
to
b4143f1
Compare
|
This is testing beautifully. Looking at the code now 👀 |
andrewserong
force-pushed
the
try/add-clipboard-events-to-the-list-view
branch
from
b4143f1
to
75d06fa
Compare
|
Thanks for taking a look! I've just given this a rebase to keep things current now that #56625 has landed. |
|
Flaky tests detected in 68ed6c7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7621336864
|
noisysocks
reviewed
Jan 23, 2024
There was a problem hiding this comment.
Tests really really well, like I said 👍
How do you feel about extracting some of the common parts of packages/block-editor/src/components/list-view/use-clipboard-handler.js and packages/block-editor/src/components/writing-flow/use-clipboard-handler.js?
Right now there's a lot of overlap which I think will bite us in the future when a contributor makes a fix or performance improvement to one hook but forgets (or doesn't know about) the other.
Perhaps both hooks could live in the same file and call the same set of util functions that do all of the block creation, serialisation, setting of clipboard data, block transform handling, block insertion, etc. Only the code that deals with block selection would differ.
| await pageUtils.pressKeys( 'primary+v' ); | ||
|
|
||
| await expect | ||
| .poll( |
There was a problem hiding this comment.
TIL about expect.poll. Just for my own understanding, what's the reason for using it here? Is it not enough to rely on Playwright's default behaviour of waiting for the selector to appear?
There was a problem hiding this comment.
That's a great question! I was just following the examples from the other tests, which use poll. In this case we're polling for an expected object that is returned by a function call, rather than for a particular selector. Internally, getBlocksWithA11yAttributes is calling getByRole etc to grab the row element itself, so I imagine this was an abstraction so that we can just focus on the shape of the block data, rather than selectors within these particular tests?
It looks like the abstraction was added in #50422 by @kevin940726. To my eyes it improves readability quite a bit, but Kai might have more context to offer 🙂
There was a problem hiding this comment.
I don't think we need it here, but it's just a preference of mine to make the assertion retryable to reduce potential flakiness.
It might be a common misconception that Playwright automatically waits for the elements when you query them, but it doesn't. It only waits for the element when you perform some "actions" on them (like clicking, or asserting). In getBlocksWithA11yAttributes we're not performing any actions so they might be outdated.
There was a problem hiding this comment.
Great, thanks for confirming the reasoning, Kai!
Co-authored-by: Robert Anderson <robert@noisysocks.com>
Oh, I like that idea, but at the same time, there's a bit of a trade-off to be made with consolidating behaviour within the writing flow and list views. When folks go to make changes to one or the other, it can be handy to know that you're not going to accidentally adversely affect the other environment, which is why I tend to lean a bit more toward duplication when dealing with the list view, as it has some nuances that we want to keep separate. Beyond block selection there's also:
But I do very much like the idea of seeing what we can split out into utility functions. If that works neatly enough, perhaps we can have cleaner to read hooks that could help reduce some of the duplication while still having separate logic where needed 🤔 I'll have a hack around tomorrow and see what's feasible! |
|
Thanks @noisysocks! I struggled a bit at first trying to consolidate between the two hooks, but the low hanging fruit wound up being consolidating the logic inside the copy and paste handling, so I've created a couple of utility functions in 885b147 for the bits that could be consolidated without creating messy function signatures. From a quick re-test, I don't think I've broken anything in the process, but it'd be good to give this another whirl to be sure 😄 |
noisysocks
approved these changes
Jan 24, 2024
packages/block-editor/src/components/list-view/use-clipboard-handler.js
Outdated
Show resolved
Hide resolved
|
Thanks for all the help, Rob! |
25 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of: #49563
Allow clipboard events within the list view to cut, copy, and paste blocks via keyboard shortcut.
Why?
When focus is within the list view, it can be very helpful to quickly copy blocks, or paste blocks over particular selected blocks. This PR allows that to happen.
How?
Make a copy of the
useClipboardHandlerand attach it to the list view so that it can handle copying, cutting, and pasting both individual and multi-selections of blocks.The copied
useClipboardHandlerdiffers from the one used in the editor canvas in some key ways:trunkwhen pressing backspace to delete blocks.Testing Instructions
Screenshots or screencast
2024-01-18.11.46.00.mp4