-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Refine overlay edit button UI: move to link variant and add tooltip #73957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
|
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: -17 B (0%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
This is better for me. I wonder if we have similar patterns in other places that we can align on. |
Yes I was wondering about that. I looked in Storybook but didn't see anything in DataForms or anywhere that had a similar UI/UX. I wonder if @jasmussen has come across anything or has any better ideas? |
|
Flaky tests detected in 4c68f67. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20162835667
|
|
We do: I've no strong opinion on what button style it should be, just that it should ideally be the same, one or other direction. I'm leaning towards tertiary, but if @jameskoster has the capacity to chime in, he'd know with certainty. |
mikachan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good and tests well for me.
However, I'm struggling to find any other similar buttons to this design, other than the example @jasmussen gave.
I'm not opposed to this design, but another option may be to use a hamburger/dropdown menu to contain the Edit link, similar to the one for the Menu tools:
I think I prefer the simple "Edit" link though, and I like that it's a clear action rather than being hidden behind another menu. Not strongly opinionated either way, and will defer to design on this!
| } | ||
|
|
||
| .wp-block-navigation__overlay-edit-button { | ||
| margin-top: $grid-unit-10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think margin-top is needed if the button is absolutely positioned, or we should instead adjust the top positioning rather than using margin.
|
I think what I'm saying is that design wise, there's an existing pattern, but if the existing pattern is poorly implemented, we can improve it here, but also ideally follow up and improve the existing implementation. Is that fair? |
|
💯 This is an opportunity to
|
|
@jasmussen noticed a "blink" of the admin when |
|
Steps to reproduce the "blink":
Crucially, you have to reload the page between applying and clicking edit on the overlay button, so there's probably some caching going on. Note also that this was inserting a navigation block in the post editor (which is already a rarity), not the site editor, that may play a role too. |
|
@getdave I'd rather accept that the help text doesn't span than hack the component If the UI component is not meant to offer this UI. |
|
I've discussed this away from Github with @mirka and the proposed design isn't compatible with our current forms components. To implement the proposed would require rebuilding It's not simple to modify the existing components with new extensions points so we may need to wait on the next iteration of form components to provide greater flexibility. @mirka may have more information on that. I propose (as per @youknowriad's comment) that we merge this PR with the compromise layout (help text not wrapping) and then circle back later (pre release) to implement the fully refined design. |
- Replace absolute positioning with HStack/FlexBlock/FlexItem layout - Change button variant from link to secondary - Add 24px top margin to align button with SelectControl input - Remove absolute positioning CSS rules - Use CSS variables for spacing
Change help text from 'Select an overlay to use for the navigation.' to 'Select an overlay for navigation.' to avoid text wrapping and improve UI appearance.
Update the test expectation to match the new shorter help text 'Select an overlay for navigation.' instead of the previous longer version.
|
I've updated this:
|
Brings in fix from commit f4d87b4 (PR #73919). When a selected overlay template part is deleted, the Edit button was still visible even though the template part no longer exists. This fix ensures the Edit button only appears when: - overlay attribute has a value - template parts have been resolved (hasResolved) - the selected template part exists in the resolved list This prevents the Edit button from appearing when the overlay selector correctly shows 'None (default)' after deletion.
MaggieCabrera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working as intended and the code makes sense to me. I'm approving tentatively but I'm unsure of the design. I googled how to translate Edit into German and the word is quite much longer, I'm not sure of this design pattern personally
jeryj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with suggestions for cleanup. I don't see any blocking issues.
| help={ | ||
| overlayTemplateParts.length === 0 && hasResolved | ||
| ? __( 'No overlays found.' ) | ||
| : __( 'Select an overlay for navigation.' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update this text in a follow-up. "Select an overlay for navigation." doesn't really explain to what the selection is going to do. It seems clear that I'm selecting an overlay, but I don't know what it's going to. I'd appreciate more context about that, like, "Customize the overlay appearance"
| const isEditButtonDisabled = | ||
| ! overlay || | ||
| ! parsedTemplatePart || | ||
| ! hasResolved || | ||
| ! selectedTemplatePart || | ||
| ! onNavigateToEntityRecord || | ||
| isResolving; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need ! hasResolved and isResolving? Also, the Edit button only renders in this condition: { overlay && hasResolved && selectedTemplatePart && ( so maybe we can just put this directly in the button?
aria-disabled={ onNavigateToEntityRecord }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh some of this disable logic might be redundant now the design has changed.
| aria-label={ editButtonLabel } | ||
| label={ editButtonLabel } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label already sets the aria-label. You can remove the aria-label.
| .wp-block-navigation__overlay-edit-button { | ||
| margin-top: $grid-unit-10; | ||
| width: fit-content; | ||
| margin-top: $grid-unit-30; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could <Flex/> or <HStack/> handle this layout instead of adding CSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No not with the HTML forced upon us by form components. This was a last resort.
|
Let's get these conflicts resolved and cleanup the final points that @jeryj suggested. We should bring this in. |




What
Refines the overlay template selector UI by moving the edit button to a more streamlined position and adding a tooltip for improved clarity.
Why
The previous implementation had the edit button as a large isolated button below the input, which created visual separation and took up unnecessary space. Moving it to a link variant positioned to the right of the control creates a more compact and intuitive UI. Additionally, the button's purpose wasn't immediately clear to all users since it only showed "[Edit]" as visible text, with additional context only available via aria-label for screen readers.
How
editButtonLabelmemoized value for both the tooltip and aria-labelScreenshot
Testing
trunk.