Consolidate disparate "copy block" actions #23088
Conversation
|
Could we update all copy buttons to use the "snackbar"? |
packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
Outdated
Show resolved
Hide resolved
ntsekouras
force-pushed
the
fix/consolidate-disparate-copy-block-actions
branch
from
1a79894
to
d735e8e
Compare
ntsekouras
changed the title
Refactored BlockActions component to use hooks
ntsekouras
force-pushed
the
fix/consolidate-disparate-copy-block-actions
branch
from
d735e8e
to
dcd525a
Compare
github-actions
bot
added
the
First-time Contributor
|
Congratulations on your first merged pull request, @ntsekouras! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
| @@ -15,7 +15,7 @@ import { __, sprintf } from '@wordpress/i18n'; | |||
| */ | |||
| import { getPasteEventData } from '../../utils/get-paste-event-data'; | |||
|
|
|||
| function useNotifyCopy() { | |||
| export function useNotifyCopy() { | |||
There was a problem hiding this comment.
With useNotifyCopy being used in more than one component, is copy-handler still the right place for its definition? (Also, is this hook a good abstraction to begin with? 🙂)
There was a problem hiding this comment.
Hey Miguel, thanks for the feedback!
useNotifyCopy could certainly go to a different file, under copy-handler folder ( even if it wasn't reused ) and maybe renamed to reflect that handles cut notifications too.
I believe it's a good abstraction as it does one thing: notifies the user of a copy/cut action. Every such action should have consistency about the way a user is notified.
| if ( blocks.length === 1 ) { | ||
| flashBlock( selectedBlockClientIds[ 0 ] ); | ||
| } | ||
| notifyCopy( 'copy', selectedBlockClientIds ); |
There was a problem hiding this comment.
Have you weighed the benefits of combining notify and flash somewhere — maybe at the action level?
There was a problem hiding this comment.
Of course I thought about combining them, but since flashBlock is used only if one block is copied/cut, didn't seem to me so consistent with the handling of the same actions with more blocks. So I think that CopyHandler might need more thought about this consistency in the future.
With the above said, I believe I need some more experience with the project, to understand it better and feel more comfortable before starting making more design decisions. I'm observing a bit more for now :)
Great thought provoking feedback Miguel! Really appreciate it!
Description
Resolves #23031
Two recent PRs have added ways to more conveniently copy an entire block from the editor, one using the Ctrl-C and Ctrl-X keyboard shortcuts (#22186), one using a new "Copy" menu item in the block's advanced settings (#22214).
While these achieve the same result for the user, their implementations follow different User experience on how the user is notified about the copy action.
In this PR using the "Copy" menu item no longer results in the updating the label to "Copied", but rather results in flashing the block and displaying a snackbar notification.
How has this been tested?
Screenshots
Types of changes
Checklist: