Try: Show copy shortcut in block options. #60339
Merged
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
jasmussen
added
[Type] Enhancement
|
Size Change: +1.56 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
t-hamano
approved these changes
Apr 2, 2024
|
Oh I just realized, ⌘C is not the same as "Copy styles". Copy styles actually has to be chosen separately if you just want to copy the styles. Let me see if there's an easy fix to not show the shortcut on that item. |
|
I sent the shortcut through to the CopyMenuItem so that I could attach it only to the main copy item. Please let me know if that still works for you! |
|
@jasmussen, As mentioned in this comment, I think "Copy" and "Copy Styles" are internally the same, but I don't have a strong opinion on whether the shortcut should be displayed in both or only in "Copy". The following issues exist related to this PR: |
|
Flaky tests detected in e97af03. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8520146847
|
|
That's curious! I just tried that, and first time I tried it, they didn't behave the same. But in that test, I had to also press "Allow", I wonder if that was at fault? The second time I tried just now, it did indeed work as you suggested:
Would you prefer I revert the latest change, or can we ship as-is? Visually I have a very small preference for the ⌘C showing up only once, but happy to rewind a bit. |
Admittedly, pasting blocks and pasting styles are different. Pasting styles may need to be allowed on the browser side depending on the environment: gutenberg/packages/block-editor/src/components/use-paste-styles/index.js Lines 143 to 151 in 6f2e542 But both Copy and Copy Styles should be exactly the same. This is because both actions call the same
I think it's fine to ship as-is. I can't remember right away, but I remember seeing a comment mentioning a problem where users get confused if the same shortcut appears in different menus. Ultimately, I believe the block options menu needs to be comprehensively improved based on the points discussed in #58650. |
|
Great notes as always, thank you. Okay, it sounds like this one is ready to land. We're in-between major releases, this is a small tweak, and I'm very happy to follow up if anything comes up. So I'm tentatively going to land this one, and we'll take it from there. |
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
* Try: Show copy shortcut in block options. * Add shortcut to only the main copy item.
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?
I noticed we don't show the ⌘C shortcut for the block Copy option in the block options:
This PR adds that:
It's not clear to me the code for showing this is right, so would appreciate a code check. Feel free to push directly to this branch.
It also has the side effect of showing this same shortcut for "copy styles" — probably technically correct, since it's the same action, but worth noting.
Testing Instructions
Select the block options button in the block toolbar. The ⌘C (or your operating system modifier, e.g. CTRL) shortcut should show up.