Try: Polish link/unlink buttons #43802
Conversation
jasmussen
added
the
[Feature] UI Components
jasmussen
requested review from
mirka,
ciampo,
glendaviesnz and
aaronrobertshaw
| @@ -6,20 +6,20 @@ import { link, linkOff } from '@wordpress/icons'; | |||
| import { __ } from '@wordpress/i18n'; | |||
|
|
|||
| export default function LinkedButton( { isLinked, ...props } ) { | |||
| const label = isLinked ? __( 'Unlink Radii' ) : __( 'Link Radii' ); | |||
| const label = isLinked ? __( 'Unlink radii' ) : __( 'Link radii' ); | |||
There was a problem hiding this comment.
Sentence case across all of these.
| iconSize={ 16 } | ||
| aria-label={ label } | ||
| /> | ||
| <span> |
There was a problem hiding this comment.
The extra span, like it does for the other instances of this button, makes the button "not have text", which invokes the :not(.has-text) CSS.
There was a problem hiding this comment.
I'm not sure I follow here — how can a parent span influence the has-text classname on the Button component?
There was a problem hiding this comment.
I'm also unclear as to why this span wrapper is required. It looks like it's been copied from the approach in the BoxControl or because the new spacing presets UI needed one? Removing the wrapper didn't appear to make any visual change to me.
With span |
Without span |
|---|---|
 |
 |
If it isn't required, I'd vote to clean that up as well. Seeing if the other components (spacing presets and box control) still need the spans wrapping their buttons could be a follow-up though.
There was a problem hiding this comment.
Inded that's curious. It was definitely needed at one point, but it doesn't appear to be anymore. Removed!
There was a problem hiding this comment.
Actually my mistake, I removed it only from one of them. When I removed it from the border version, this happened:
Let me look at a different solution than adding a span.
There was a problem hiding this comment.
In that new PR, we could even include these changes — so that we keep that PR focused on the Button component
There was a problem hiding this comment.
Should we just include it in this PR? Otherwise can we land this one with the span intact (PR as-is now), and then remove the span in that new separate PR?
There was a problem hiding this comment.
I'm asking because it's pretty crucial we get this PR merged before the freeze, so anything we can do to land this one sooner rather than later would be great. The "has-text" issue seems like a bug that can be fixed after the freeze as well.
There was a problem hiding this comment.
I will begin work on a PR for the button.
If @jasmussen's PR should be merged first, I think there is no problem with reinstating the span tag.
I think it's not too late to decide whether to remove span tag or not after the two PRs have been merged in the end.
There was a problem hiding this comment.
I have submitted #44198, but I need to do further validation because some unit tests are failing 😅
I vote to proceed with this well-reviewed PR merge.
| isSmall | ||
| icon={ isLinked ? link : linkOff } | ||
| iconSize={ 16 } | ||
| iconSize={ 24 } |
There was a problem hiding this comment.
In general icons should never be smaller than 24px, with almost no exceptions, because it causes blurry antialiasing of the pixels that were designed for the 24px grid. The only exception I can think of, is the external link icon, which can occasionally be shown in 12px size. But at that size (exact half), the pixels interpolate correctly, so it remains crisp.
| width: 24px; | ||
| padding: 0; | ||
| width: $icon-size; | ||
| min-width: $icon-size; |
There was a problem hiding this comment.
Style changes in this file are probably the most impactful, so it would be good to test small icon buttons with and without text.
There was a problem hiding this comment.
This CSS seems to affect buttons that have isSmall and icon prop and no text.
For one, it changes the width from 36px to 24px.
This is because the trunk applies min-width: 36px; even if the button is small:
gutenberg/packages/components/src/button/style.scss
Lines 305 to 307 in 9a345b7
Second, the SVG icon size is different. In the trunk, the SVG icons are squashed into 8px padding on the left and right, and displayed at 20px. (36px - 8px * 2 = 20px)
In this PR, the icon size is 24px.
However, I think this is not the intended behavior in trunk, and the change made by this PR is expected behavior.
Trunk
This PR
There was a problem hiding this comment.
Thank you for looking, @t-hamano, much appreciated. Inded, the intended behavior is for the icon to always be 24x24. The screenshot of this branch above looks like the correct behavior to me.
|
Size Change: -58 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for iterating on these buttons @jasmussen 👍
Unfortunately, I ran into a few issues while testing this PR. I've added inline comments with more details but the TL;DR is:
BorderBoxControlheight has been increased due to new styles also throwing off vertical alignment.- Buttons with
isSmalland text are missing their horizontal padding e.g.BoxControlreset button, Image block sizes etc. - We also need a changelog for any changes to controls in our components package.
| line-height: 22px; | ||
| padding: 0 8px; | ||
| padding: 0; |
There was a problem hiding this comment.
Did you mean to remove the horizontal padding here? I noticed this affects the reset button for the BoxControl.
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
This also breaks other isSmall buttons in the editor such as those for the image block's sizes etc.
There was a problem hiding this comment.
Thank you for excellent feedback and thank you for looking. It does seem like this PR is unlikely to land as is.
With this PR in mind as an example, is there a different and safer approach we can take, to ensure the link/unlink button can move forward, i.e. componentization? Or do you still see the iterative approach as taken in this PR as potentially feasible?
There was a problem hiding this comment.
With this PR in mind as an example, is there a different and safer approach we can take, to ensure the link/unlink button can move forward, i.e. componentization?
I've actually made the argument internally as well that these linked buttons would be good candidates to make into a reusable component when bandwidth allowed. I don't think doing so moves the redesign of these buttons forward any faster, in fact, likely the opposite.
Or do you still see the iterative approach as taken in this PR as potentially feasible?
The iterative approach proposed in this PR makes sense to me as I don't believe it is too far off.
The style accidentally increasing the BorderBoxControl's height can be fixed easily and with a slight change in approach to tweaking the linked button's padding we should be very close to landing this.
There was a problem hiding this comment.
Cool thank you, I'll see if I can polish this along as I get a breather! 🙏
|
Should the icon be the other way around? IE, when the sides are linked, the button label should be the unlink icon since that describes what will happen on click? |
|
Showing the full link when sides are linked seems to be the standard in Photoshop, Illustrator, and Figma:
|
ciampo
added
[Type] Enhancement
jasmussen
force-pushed
the
try/link-unlink-polish
branch
from
38aeedf to
22c932c
Compare
|
Alright, thanks to excellent sleuthing by @aaronrobertshaw, I think this one is ready for another review:
|
|
Cool, I'll wait for Aaron to sanity check and then land this! 🙏 |
There was a problem hiding this comment.
Thanks for iterating on this PR @jasmussen 👍
It looking and testing better for me now.
✅ Issue with increased BorderBoxControl height has been resolved
✅ Horizontal padding has been restored to isSmall buttons with text e.g. width buttons
✅ Couldn't find any other instances of isSmall buttons that looked off
❓ There appears to be an unnecessary span wrapping the border radius button as Marco highlighted
If we can remove the span noted above I think this is almost ready to merge. It might pay to double check with @t-hamano regarding the icon sizing behaviour he raised and confirm we're happy to proceed.
Screen.Recording.2022-09-15.at.4.37.11.pm.mp4
| iconSize={ 16 } | ||
| aria-label={ label } | ||
| /> | ||
| <span> |
There was a problem hiding this comment.
I'm also unclear as to why this span wrapper is required. It looks like it's been copied from the approach in the BoxControl or because the new spacing presets UI needed one? Removing the wrapper didn't appear to make any visual change to me.
With span |
Without span |
|---|---|
|
|
If it isn't required, I'd vote to clean that up as well. Seeing if the other components (spacing presets and box control) still need the spans wrapping their buttons could be a follow-up though.
There was a problem hiding this comment.
Jumping in because Marco has wrapped up for the week — I'm also fine with merging this PR with the temporary span in place 👍 Maybe add a // TODO: Remove span after merging https://github.com/WordPress/gutenberg/pull/44198 or something like that so we don't forget.
We should also add a changelog for this size change because I imagine a lot of consumers have added their own style overrides to fix this.
jasmussen
added
the
Backport to WP 6.7 Beta/RC
|
Thank you! I've kept the spans in place, but added a comment. @aaronrobertshaw if you want a last look, I'd appreciate it, otherwise I'm going to land this one when the tests pass. |
Co-authored-by: Aaron Robertshaw <[email protected]>
jasmussen
force-pushed
the
try/link-unlink-polish
branch
from
e290a4e to
2b90bd0
Compare
There was a problem hiding this comment.
@jasmussen Thanks for the ping and seeing this one through 👍
I've given this a quick test and it's working well for me.
We still need the a changelog as noted in a few earlier comments before we merge this one though. Just a quick note detailing the changes to the BorderBoxControl, BoxControl, and Button components is all that's needed.
Once that changelog is in place and tests are all green, merge away! 🚢
|
Ack my apologies, I forgot to update the changelog (long week). Done now, let me know if it needs changing. Thanks everyone, and I'm happy to land this. 🙏 |
* Border link button. * Padding & text case. * Radius. * Update packages/components/src/border-box-control/styles.ts Co-authored-by: Aaron Robertshaw <[email protected]> * Restore is-small padding, and fix spacing between unlink and custom buttons. * Remove span again. * Keep spans in place but add a comment. * Update changelog. Co-authored-by: Aaron Robertshaw <[email protected]>
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 810ba68 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
What?
The link/unlink buttons used across the interface are blue, rectangular, and uses a scaled and blurry/illegible icon:
This PR changes those to be plain 24x24 icon buttons toggles:
Why?
Instead of having a separate style just for link/unlink buttons, this focuses in on an existing icon button style already in use, it reduces prominence, and it makes the icon more legible.
How?
The PR changes the style of the buttons in a few places, and it touches some legacy inherited CSS. It seems like there's an opportunity for a singular link/unlink component, but this at least takes some steps towards it.
Testing Instructions
Please test border, radius, padding controls and observe the link/unlink button being correctly scaled (24x24) and positioned.
Because this touches on some older styles for "small" buttons, it would be good to look up any instances of buttons with text that use the isSmall prop, and see that those still look correct.