Repository navigation
Add 'Reset' option to MediaReplaceFlow component - #64826
Merged
Merged
Conversation
…ured-image blocks
akasunil
added
[Type] Enhancement
|
Size Change: -354 B (-0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
…enhancement/MediaReplaceFlow-reset-option
…enhancement/MediaReplaceFlow-reset-option
|
Flaky tests detected in 0c3716a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10590857495
|
akasunil
marked this pull request as ready for review
August 28, 2024 10:48
akasunil
requested review from
ajitbohra,
artemiomorales and
ellatrix
as code owners
August 28, 2024 10:48
|
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. |
Contributor
|
Nice, this feels like a good cleanup and optimization. I think this just needs a code review. Great PR! |
Mamaduka
reviewed
Aug 28, 2024
Comment on lines
-71
to
-75
| mediaUrl={ | ||
| useFeaturedImage && featuredImageURL | ||
| ? featuredImageURL | ||
| : mediaUrl | ||
| } |
Member
There was a problem hiding this comment.
This looks unrelated. Can you explain why we're removing special handling for featured images?
Member
Author
There was a problem hiding this comment.
This condition is already handled here when passing argument to ToolbarEditButton. Don't see any need to add same condition again in ToolbarEditButton component.
Mamaduka
reviewed
Aug 28, 2024
Comment on lines
+436
to
+437
| closeAndFocus(); | ||
| onResetImage(); |
Member
There was a problem hiding this comment.
Besides this component and the Cover block, all reset callbacks are the same as onSelect( undefined ). I wonder if we really need a new onReset callback.
Member
Author
There was a problem hiding this comment.
We can make it optional by setting onSelect( undefined ) as default action for Reset option.
Member
There was a problem hiding this comment.
That adds more logic that MediaReplaceFlow needs to handle, and I was thinking the opposite; it already supports too many "on-action" callback props.
@ramonjd, @andrewserong, what do you think?
Member
There was a problem hiding this comment.
I'm torn.
A benefit is that consumers don't have to include their own <MenuItem /> and onClose() functionality. I see that, in trunk, the menu hangs after reset
The downside is that the "Remove" item won't be customizable for each block or file type. What if, later, the copy needs to update, e.g., Remove [specific item] or extra accessibility props — then we'd be loading <MediaReplaceFlow /> again. For that reason this optimization could be premature.
Member
There was a problem hiding this comment.
and onClose() functionality
Child functions get passed that any way I see:
Member
There was a problem hiding this comment.
After adding "Reset" support to most media blocks in the core, I see this as more consolidation than optimization.
I'm pro-composition, but here, it seems we want to have the same feature/label across the blocks.
Contributor
There was a problem hiding this comment.
Good discussion! I suppose one potential challenge with removing the onRemove prop and consolidating around it calling onSelect( undefined ) is that the MediaReplaceFlow component appears to be a public / stable one. Would consumers be guaranteed to handle it, i.e. what if someone has a callback that always expects a value? And should consumers always have a Reset option?
I suppose one of the neat things about the onRemove prop is that if consumers don't provide a callback, the option will never be displayed, so it's backwards compatible in case anyone has code that wouldn't handle onSelect( undefined ) gracefully.
But I don't feel too strongly either way on this topic 🙂
Member
There was a problem hiding this comment.
Thanks, good point, @andrewserong!
If consumers don't expect the "Reset" button, the block editor shouldn't surprise them with one :)
Having a new onReset prop and conditional rendering will allow us that.
Mamaduka
approved these changes
Aug 29, 2024
Member
Author
|
Thank you @Mamaduka, @ramonjd and @andrewserong. I'll Merge this PR now. |
bph
pushed a commit
to bph/gutenberg
that referenced
this pull request
Aug 31, 2024
* Add reset option in MediaReplaceFlow component * Add media reset option in toolbar of media-text block * Refactor MediaReplaceFlow component use in cover, image and post-featured-image blocks * Refactor MediaReplaceFlow component use in site-logo block * Refactor BackgroundImageControls component under global styles * Add media reset option in video, audio and file blocks * Fix onReset argument value error on image block * Remove unused MenuItem component from blocks * Reset attribute on reset option click in file block * Fix issue of reset option in media-text block Co-authored-by: akasunil <sunil25393@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
This file contains hidden or 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
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? and Why?
Resetoption is used to clear media from block, such as image block, cover block etc. However, theMediaReplaceFlowcomponent does not by default have theResetoption available. We used to add theResetoption as a child of theMediaReplaceFlowcomponent, Whenever necessary. We had to include theResetoption in theMediaReplaceFlowcomponent itself because it is a fundamental feature that is needed anywhere media is used, saving us from having to write additional code.How?
This PR,
Resetoption toMediaReplaceFlowComponent.MediaReplaceFlowcomponent inImage,Cover,Post Featured Image,Site LogoblocksResetoption toAudio,Video,FileandMedia-Textblock.MediaReplaceFlowcomponent in background support under global styles.Testing Instructions
Image,Cover,Post Featured Image,Site Logo,Audio,Video,File,Media-Text)Resetoption is visible in toolbarResetTesting Instructions for Keyboard
Screenshots or screencast