Made SiteLogoReplaceFlow always available in the Site Logo block toolbar #63499
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
richtabor
added
[Type] Enhancement
|
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. |
|
Size Change: +709 B (+0.04%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
mtias
reviewed
Jul 12, 2024
t-hamano
reviewed
Jul 13, 2024
+ show "Reset" when there is a logo uploaded
richtabor
force-pushed
the
tweak/site-logo-persist-replace-flow
branch
from
4e21760 to
089de65
Compare
t-hamano
reviewed
Jul 23, 2024
There was a problem hiding this comment.
One thing that bothers me a bit is that when I press the reset button only the reset button disappears but the popover remains open.
14651ab4a6b4bea6235c47f07be15619.mp4
I would expect the popover to disappear when reset, just like with the sidebar.
If the ideal behavior is for the popover to disappear when you press the reset button, I'd be happy to help you update the code to achieve that.
Agreed. Sure thing, I'd appreciate it. |
t-hamano
reviewed
Jul 24, 2024
Comment on lines
506
to
511
| // Focus the toggle button and close the dropdown menu. | ||
| if ( ref.current ) { | ||
| const [ toggleButton ] = focus.tabbable.find( ref.current ); | ||
| toggleButton?.focus(); | ||
| toggleButton?.click(); | ||
| } |
There was a problem hiding this comment.
I added this code to fix the issue pointed out in this comment. This code ensures that when you press the Reset button, the dropdown closes and focus returns to the block toolbar button.
Before
before.mp4
After
after.mp4
@Mamaduka I've used the code here as a guide, but do you know of any other good approaches?
There was a problem hiding this comment.
Maybe we should update the MediaReplaceFlow to support the "function as a child" component pattern. Then, we can pass onClose as a prop and let the dropdown in MediaReplaceFlow handle focus for us. The media upload buttons handle focus return using the same method.
I've proof of concept code that I can push on this branch.
There was a problem hiding this comment.
Thank you for your advice.
Do you mean to change this line to the following?
{ typeof children === 'function'
? children( { onClose } )
: children }There was a problem hiding this comment.
@t-hamano, did I assume correctly that you want to work on the suggested changes?
There was a problem hiding this comment.
Yes, I would be happy to work on the suggested changes. May I submit that in a separate PR?
There was a problem hiding this comment.
Sure. What works best for you 👍
|
In this PR, I updated the children of the a73f81ad57532ddd7b7a9e6c5c0f98ae.mp4I also added a note about the changes to |
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.
Makes the SiteLogoReplaceFlow always available in the toolbar, to render "Choose Site Logo" when there is no logo, and "Replace" when there is one. Part of #63327.
The "Choose Site Logo" string should map to the same as what we decide for the Inspector, as I explored in #63498.
Why?
It should be clearer how to add a site logo and using an existing affordance that we apply elsewhere is much more supportive of the user experience.
Testing Instructions
Screenshots or screencast
Added by @t-hamano
The
SiteLogoReplaceFlowcomponent is a wrapper component forMediaReplaceFlow, and the Reset button is rendered as a child ofMediaReplaceFlow. When the Reset button is pressed, it is expected that the popover will be closed and the focus will return to the block toolbar button, but currentlyMediaReplaceFlowcannot callonClosefrom the child.Therefore, in this PR, we will update
MediaReplaceFlowto support "function as a child". function as a child can receiveonCloseas a prop.