Improve implementation of the 'Show button text labels' preference #61763
Comments
afercia
added
[Type] Enhancement
|
Here's the occurrences of the CSS selector |
|
Excited to see this! I’ve always wanted that setting to work more like MacOS toolbars. |
|
The more I investigate this issue, the more I tend to think any CSS implementation is inherently fragile and not solid enough to make this feature reliable. Also, it's not well testable. The current implementation uses a CSS pseudo element
Other potential issues may arise from other potential CSS conflicts. Fudnamentally, revealing the text by using CSS pseudo elements that target the button However, I do realize the base Button component should not support this as a built-in feature. I can think of two options and I'd greatly appreciate some feedback from the Components package maintainers and other Gutenberg specialists:
The second option seems cleaner ot me and wouldn't require any CSS hacks. |
|
#46025 proposed not relying on global class names. One approach I can think of is to change the behavior of the function ParentComponent() {
const showIconLabels = useSelect( ( select ) => {
return select( preferencesStore ).get( 'core', 'showIconLabels' );
}, [] );
return (
<>
<ChildConponentOne showIconLabels={ showIconLabels } />
<ChildConponentTwo showIconLabels={ showIconLabels } />
</>
);
}
function ChildConponentOne( { showIconLabels } ) {
return (
<Button
label={ __( 'Push Me' ) }
icon={ showIconLabels ? someIcon : undefined }
showTooltip={ ! showIconLabels }
>
{ showIconLabels && __( 'Push Me' ) }
</Button>
);
}
function ChildConponenTwo( { showIconLabels } ) {
return (
<Button
label={ __( 'Push Me' ) }
icon={ showIconLabels ? someIcon : undefined }
showTooltip={ ! showIconLabels }
>
{ showIconLabels && __( 'Push Me' ) }
</Button>
);
}However, this will require changes to all UI that currently depend on the |
We can accomplish this using the Context System. For what it's worth, I'm not against this approach at all. On the other hand, the latter approach with the Editor-only wrapper seems unfeasible because we'd have to replace all relevant I think the most difficult part would be the reliability of the automatic transform. Because |
Yes I have the same concern as well. For this reason I'd tend to think the base component should be kept unchanged. The wrapper component option seems more reasnbale to me as it addresses a feature that is specific to the Gutenberg editor.
Yes. I'm not sure it's a lot of files to touch, actually I have the impression it's a few files overall. Some of these compound component already use an I'll try and submit a Draft PR so to have some proof of contept to look at. |
github-actions
bot
added
the
[Status] In Progress
|
I just reported a new regression for this preference, which proves once more the current implementation is fragile. On latest trunk the Inspector tabs text is no longer visible. See #61947 |
|
#61947 / #61949 is a good example of how the current implementation is fragile and prone to break when some controls styling is changed without appropriately testing the consequences of those changes. To me, such regressions prove a few things:
|
|
@afercia agree on all counts. I believe @mirka created (or at least at some point showed me) some sort of decision workflow related to whether certain styles or features should be built into components vs. implemented at higher layers through escape hatches or overrides. Can we get that linked here? |
|
Yes it would be great to have a look at that decision tree. I would say that, since this is a collaborative open source project, such a decision tree should be documented. Regarding 'escape hatches or overrides', in general, based on my experience, proliferation of ad-hoc 'local' styling leads to UI inconsistencies and higher maintenance cost in the long run. This is typical in WordPress core, as it 'accumulated' a couple decades of development. I would love to see Gutenberg not taking the same path. I'd think the editor should be very very strict and avoid ad-hoc styling as much as possible. For example, if a Button gets some custom ad-hoc styling for some design purposes that is only decorative and only for one case then I'd think introducing such a customization isn't worth it. On the other hand, when some custom styling proves to be beneficial and has multiple use cases, then it probably worths a variant to be added to the base component. IMHO, a good example of what not to do si the cystom styling for the featured image buttons in the Post tab of the Inspector:
As far as I can tell, that's a unique styling and it's not used anywhere else in the UI. To me, it just introduces inconsistency and doesn't add great value. Design should avoid to introduce such unique customizations. |
I'm sorry but I have no idea what you're referring to 🫣 Maybe I was talking about the general principle that
To be fair, I can't remember a time when @ciampo or I ever tested for "Show button text labels", nor had it been escalated to us as a QA issue. I was passively aware of the feature, but it was never in our smoke testing routine. From the maintainability standpoint, I think we need to improve the "Show button text labels" implementation to either be:
Given that "Show button text labels" breakage in a practical sense can also be dependent on the surrounding UI context, I think the latter would be ideal. |
Description
The 'Show button text labels' preference is an usability and accessibility features that makes icon buttons show visible text instead of icons.
It's not just accessibility. It's general usability, as some users may just prefer visible text. It has been inspired by the similar macOS finder preference, where users may choose to show buttons with: Icon and text, Icon only, Text only. Screenshot:
So far, this feature isn't 'built-in' in the components. Rather, it is implemented by the means of 'local overrides' of the buttons styling. It is also worth reminding that it is not implemented for all buttons but only for some of them. The related CSS is scattered all around in several files and compoennts. It's difficult to maintain and it's duplicated code.
Over time, this feature faced several breakages as it is often missed in the design of new features in the first place and often missed during developing and manual testing. #61761 is the most recent breakage.
The
Buttoncomponent isn't aware of this preference, as it's more a preference of the environment the component lives in. As such, an architectural choice should me made first on whether the Button componetn should support this feature natively or keeping the current approach based on local overrides. As I see it, considering the several breakages in the history of this project related to this feature, the former option would be preferable.I would appreciate any thoughts and feedback on the best way to make this feature more solid, incorporated in any new design, and better testable.
In the meantime, I'd think there's a few points that can be addressed soon to make this feature better meintenable. For example, looking at the CSS used for local overrides, there's a few things that are duplicated all around and could use a mixin:
What the various CSS overrides always do is basically the following:
aria-labelattribute.auto.As a start, a new mixin could be used to have this rule in a centralized place and avoid code duplication. The mixin could be then extended to incorporate other styling, as much as possible. Small adjustments on a per component base would be fine, I guess, but the main rules for the styling should be centralized in an unique place.
Step-by-step reproduction instructions
N/A
Screenshots, screen recording, code snippet
No response
Environment info
No response
Please confirm that you have searched existing issues in the repo.
Yes
Please confirm that you have tested with all plugins deactivated except Gutenberg.
Yes
The text was updated successfully, but these errors were encountered: