Try: Update Tertiary Button appearance #48888
Conversation
jameskoster
added
General Interface
|
Size Change: +69 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a742a6d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4367085688
|
|
The save disabled draft and preview buttons are acting differently, I suspect from CleanShot.2023-03-07.at.10.44.29.mp4Also noting how metrics will change in other areas we have double tertiary buttons: Before:
After:
|
Do you think this should be addressed at the component level, or contextually? I don't find the space between a Primary and Tertiary to be problematic:
It's only consecutive instances of Tertiary buttons that feel too spaced out. Sidenote: It's interesting that we use Tertiary buttons in some placeholder and Secondary ones in others 🙈
|
Maybe at the component level, if there's two tertiary buttons side-by-side. The oddity with the draft/preview buttons is that when they're disabled, they will show very close together (as there's a background color).
Yea I noticed that as well... |
I just noticed that we have a |
|
Thanks for taking a stab. Here's a before:
Here's after:
Overall, the new hover style is a vast improvement, I love it, let's at least land that. It will tie well into the hover styles in the inserter as well. But the padding change really isn't working for me. 90% of the time, the tertiary/text buttons will be in their resting state, and without the adjusted padding, the spacing will look wrong. To be clear, I also think we need to update the disabled state. We already have a disabled state for the "✅ Saved" button, I don't see a reason the default button can't use the same visual appearance:
Having reduced padding on the text only button is also going to be important for modals and dialogs where the confirm/apply button a text only. The larger the padding, the more it will misalign with the X. Google Material is not a template for the work we do here, but it's useful to share how they are also applying a smaller padding for text-only buttons:
|
Is this a convention we want to install? Iirc all our modals currently have a Primary / Tertiary combo. Indeed the unequal padding there was part of the inspiration for this PR (Cancel button is hovered):
I suppose it's subjective, but that doesn't look right to me. That said, I'm happy to make this PR purely about the hover state, and tackle the dimensions / active states separately. |
To me what stands out there is really the hover style, not the padding. As for whether we'll ever have a text only "confirm" action in a modal? Probably not, but in the material documentation it seemed like a valid use case to consider:
In any case, this to me just affirms that text buttons just work well when their padding are balanced towards their optical footprint. I would avoid any padding-changing mechanics of adjusting it only when they are shown as sequences. A text only button on its own still feels like it should have its padding adjusted. |
|
That's the tricky thing in my mind, the optical footprint can change based on the situation, and there's no way for the atomic component to inherently interpret that. In your example above the text button either needs 0 padding, or a negative offset to align nicely with the preceding text. That detail would be the responsibility of a CardActions (or similar) component. In any case, I reduce the padding 😁 It's 8px now rather than the original 6px, to better align with our baseline grid. It seems ok, but we can go back to 6px if necessary: tertiary.mp4 |
|
I still prefer 6px 🙈 I think we are missing an opportunity to look at the padding in a different way. If focus and hover styles were pseudo elements, they could have almost whatever padding we liked, without changing the base metrics (and consequently spacing) of the text button. I know that goes beyond your effort here. |
Okay, I'll bite xD How come? If we're concerned with holistic alignment, and are committed to a 4px baseline, then 8px padding increases the likelihood that text will align naturally in certain scenarios? 6px is almost guaranteed to align with nothing. |
The minimal spacing differential doesn't fix the original concern, but blurs the lines a little bit more. Further, the 6px padding was also chosen to account for mobile, and I'd be worried about a seemingly innocuous PR addressing that. The original issue is valid, but I think we can unbundle the padding change and discuss it separately, letting the excellent hover change land. The 2px differential in this PR is not enough to address the original issue, but just blurs the boundaries more. A better solution, I really believe and want to try in a PR, is to adjust the disabled, active, and focus states to explore pseudo elements that keep the full padding of the button, even if the reduced footprint of the text button stays the same. |
|
Fair enough, I've reverted to 6px. We can now treat this PR as solely changing the hover state. |
|
Nice one! I was about to approve it, but then I realized that you are unsetting a box shadow rather than removing it at the source. I think we can probably remove it? As far as the footprint, active/focus/disabled states, let's definitely follow up separately, but agree it would be nice to just land this hover state. |
Good catch, there's probably a better way to do that. It's currently applied to both secondary and tertiary 'at source', but we can move that to the secondary style only. |
jameskoster
added
the
Design System
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
What?
Try some updates to the Tertiary Button appearance.
Why?
There are a couple of issues I'm seeking to address:
activestate features a blue-on-grey appearance which looks a little muddy.How?
Taking inspiration from the updated Block Inserter, a solid background is applied instead of an outline on hover. Additionally the padding override is removed so that Tertiary buttons share padding with all other text buttons.
It's worth noting that the padding override was originally added to improve the optics when Tertiary buttons appear consecutively, e.g. here:
But this feels like something that should be addressed at the 'organism' level rather than the component level.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast