Use block label and icon for the inserter draggable chip. #51048
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
juanfra
added
[Type] Enhancement
|
Size Change: +183 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
@ntsekouras and @jasmussen should have a better perspective on reviewing those changes :) |
priethor
added
[Type] Enhancement
|
It seems the main change to me is that instead of wrapping to two lines and saying "1 block" when inserting an image, it now has an icon and just says Image? And with icons for video as well. If that's correct, this seems good to me 👍 👍 |
|
@jasmussen thanks for looking into this. Exactly, the issue was reported by Matias, and the proposed solution also looks good to him. |
richtabor
reviewed
Jun 1, 2023
| @@ -43,6 +43,7 @@ function BlockPattern( { | |||
| isEnabled={ isDraggable } | |||
| blocks={ blocks } | |||
| isPattern={ !! pattern } | |||
| label={ __( 'Pattern' ) } | |||
There was a problem hiding this comment.
Should this always be "Pattern"?
There was a problem hiding this comment.
Good catch. I haven't thought of it because it's the pattern inserter. I'll add some changes to this.
|
Looks good to me; nice polish here. |
|
Thanks Rich, added a tiny modification to only send the pattern label when it's a pattern. It should be good to go now. |
ntsekouras
reviewed
Jun 1, 2023
There was a problem hiding this comment.
Thanks for the PR @juanfra! The new behavior is a nice improvement!
| @@ -128,6 +128,8 @@ export function MediaPreview( { media, onClick, composite, category } ) { | |||
| ( select ) => select( blockEditorStore ).getSettings().mediaUpload, | |||
| [] | |||
| ); | |||
| const blockType = getBlockType( block.name ); | |||
There was a problem hiding this comment.
We should use the getBlockType selector instead of the function. Related though to the below comment for moving the logic in BlockDraggable component.
| @@ -203,7 +205,12 @@ export function MediaPreview( { media, onClick, composite, category } ) { | |||
| const onMouseLeave = useCallback( () => setIsHovered( false ), [] ); | |||
| return ( | |||
| <> | |||
| <InserterDraggableBlocks isEnabled={ true } blocks={ [ block ] }> | |||
| <InserterDraggableBlocks | |||
There was a problem hiding this comment.
I think the retrieval of block type info could be part of BlockDraggable, like we do for the icon here.
edit
It might be needed to be moved in InserterDraggableBlocks component. I haven't checked all the internals, but my point was about moving it one level below where blocks are available.
There was a problem hiding this comment.
Ok, I'll move it. I placed it here, because the InserterDraggableBlock already had the icon parameter and I thought the expectation was to have it implemented at some point.
| isEnabled={ true } | ||
| blocks={ [ block ] } | ||
| label={ blockType?.title } | ||
| icon={ blockType?.icon } |
There was a problem hiding this comment.
If an icon is provided the label is not used anywhere, so we wouldn't need it.
There was a problem hiding this comment.
Sorry for the ignorance, but will a block always have an icon? I was thinking about extensibility and the case where some third party wants to use this, and the block doesn't have an icon.
| export default function BlockDraggableChip( { count, icon, isPattern } ) { | ||
| const patternLabel = isPattern && __( 'Pattern' ); | ||
| export default function BlockDraggableChip( { count, icon, label } ) { | ||
| const blockLabel = label && label; |
There was a problem hiding this comment.
If we would remove the isPattern, we should remove all its usages in code base. Having said that and taking into account my other comments. I think we could preserve this line here and when isPattern is true just show the label(that would mean do not prioritize any icon provided.
In general this components are public API and we should be wary of the changes we make.
There was a problem hiding this comment.
Gotcha, I'll be sure to apply those changes and revert.
My thought process here was that the draggable chip should be agnostic and, architecture-wise, should be only receiving the icon and/or label - being the pattern inserter the one in charge of sending the Pattern label. It looked to me like the isPattern logic didn't belong here.
|
Flaky tests detected in f1a7a41. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5375862643
|
richtabor
approved these changes
Jun 23, 2023
ntsekouras
approved these changes
Jun 26, 2023
There was a problem hiding this comment.
Thanks @juanfra! I pushed a minor thing to add the useSelect deps and this is good to 🚢
…e draggable chip width to respect the max-content.
ntsekouras
force-pushed
the
fix/51010-block-tooltip-when-dragging-media-into-the-canvas
branch
from
90d3bea
to
f1a7a41
Compare
ntsekouras
deleted the
fix/51010-block-tooltip-when-dragging-media-into-the-canvas
branch
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…51048) * Use block label and icon for the inserter draggable blocks. Adjust the draggable chip width to respect the max-content. * Only send `Pattern` as the label when it is a pattern. * Apply suggestions from code review. * Remove empty lines. * add deps --------- Co-authored-by: ntsekouras <[email protected]>
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?
Use the block title and icon for the draggable chip, to give context of the type of block that's being inserted.
Fixes #51010
Why?
The draggable chip wasn't giving the context of the block that was being inserted. Instead, it was showing "1 block" as text, without any icon, and it was having two lines when the string contained more than one word:
Testing Instructions
Screenshots or screencast
Before
Screen.Recording.2023-05-26.at.14.18.04.mov
After
Screen.Recording.2023-05-29.at.13.21.12.mov