Fix Global styles panel header buttons text overlap for 'Show button text labels' #63243
Conversation
afercia
added
[Focus] Accessibility (a11y)
|
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: -3 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
afercia
force-pushed
the
fix/complementary-area-header-buttons-show-text
branch
from
9164d89 to
c8af9d2
Compare
|
@WordPress/gutenberg-core I'd appreciate a review, when you have a chance. |
There was a problem hiding this comment.
It probably works fine for Latin languages, but at least for Japanese it seems to cause overflow.
In both trunk and this PR I can see extreme text wrapping.
trunk:
This PR:
One approach might be to apply white-space: nowrap to the header title (.edit-site-global-styles-sidebar__header-title):
Regarding the size of the close button, I personally lean towards keeping it at the current size (24px), since the size of the close button on the main inserter button is 24px:
However, I would be grateful for any design feedback on this point.
|
@t-hamano thanks for testing in Japanese. I'd love to see all designers and developers test often with other languages in their daily workflow. There's really no ideal solution to this layout issue other than changing a design that is problematic in the first place. The panel header can't contain all that content and the length of the content is unpredictable. The original design must be reconsidered. See #63251 What we can do now is to try the best compromise for a temporary improvement. In the last commit I've refactored a bit the flexbox implementation of this specific panel header. Is this wrapping acceptable in Japanese @t-hamano?
As I mentioned earlier, this can't be perfect for all cases but I think it's an improvement compared to trunk. |
Yes. In Japanese, there are no spaces between words, and it is quite common for words to wrap mid-word depending on the width of the container. So I think this is not unnatural and is acceptable (and probably Chinese as well). |
|
Thanks @t-hamano.
A final review and approval would be appreciated. Thanks. |
|
The revisions button invokes a drilldown, which feels strange given the access point (panel header toggle button). I wouldn't object to moving this to the panel footer, similar to the Styles sidebar: 
This would create some extra space for the other buttons which might help? |
|
Those two margin-lefts should no longer be necessary: Also, I noticed that the button is not sized correctly in the block sidebar:
Additionally, I remember that the center of this close button might be expected to be aligned with the center of the Options button:
Therefore, I lean toward leaving the button size at 24px. |
@jameskoster interesting point. To me all those buttons shouldn't be in the Styles panel header in the first place. For example, also the ellipsis button is arguably consistent. I'd expect an ellipsis button to always be the last button in a set of controls. Rearranging these buttons is out of the scope of this PR though. We agreed in past conversations to avoid scope creep so I'd encourage you to create a new issue. I adjusted the Close button size to 'small', as requested. However, I disagree with the recent change that made these buttons smaller. The target size of any control should be as large as possible, I'm not sure that intentionally (and unnecessarily) reducing the target size is a good strategy to provide a bettter user experience. I will create a new issue. |
There's also a separate expectation around where a close button is positioned. I definitely agree these are in conflict with each other and a factor in why the UI here feels a bit off. If I had to pick one to take precedence, I'd think the expectation around the close button would be strongest. So I can understand the past reasoning that lead us here.
A different perspective here (well, mine specifically 🙂) is that the scope of the PR was to fix the overlapping headings. The proposed relocation of the revisions button/options is a potential part of a solution to that. Sometimes, we can do both things i.e. be cautious of true scope creep and remain flexible. Correct me if I'm wrong but the overlapping isn't a new bug? So perhaps we can afford to take a moment to hone in on a better solution? |
Thanks @aaronrobertshaw for bringing in additional considerations. I'm not sure trying to establish which one of these buttons has precedence to be relocated as last would solve the actual issue. There are arguments for both cases. To me, relocating would cure only the symptom. The root cause is that the additional buttons should not be placed in the panel header in the first place. The panel header should conly contain:
Any other content should be moved to a 'sub-header' as proposed on #63251
Correct, it's a long standing issue. A previous PR tried to improve things but it's still not ideal. I'd like to unblock this PR to solve an issue that has been reported on May, that's a few months ago already and it's a bit too much time to fix such a small issue. Of course, this is a temporary improvement. I'd encourage further discussion on the overall design that, in my opinion, should be changed to address the root cause. |
My comment wasn't intended as a solution, just a counter point to the expectation of the ellipsis button being last. It was also aimed at understanding the problem space and the context behind the current situation.
True. We are where we are though so relocating a button to the bottom of the panel is a good first step towards an ideal scenario. Whether that is moving all buttons outside the header or into a sub-header as proposed.
Understood. Sometimes things need time though.
I agree that a holistic solution is best left for a separate discussion. My two cents on the current approach though is that while it is an improvement on the overlapping labels, is it that much extra to move the button to the bottom of the panel for a much bigger payoff? This branch could still result in overlapping labels, removing a button helps mitigate the chance of that. 
|
There was a problem hiding this comment.
Thanks for the PR! I get @jameskoster and @aaronrobertshaw points about the possibility of moving the revisions button to the bottom of the panel, but I also think this can be handled in a follow up. The main reason is that this PR improves the current situation and the moving of revisions can trigger way more discussions, as it did with post revisions.
People might want more prominence and discuss position etc..
I'll give a tentative approval and suggest to create a new issue about moving the revisions in a follow up PR. What do you say @jameskoster and @aaronrobertshaw?
Fair enough, if that was the experience of moving the post revisions 👍 |
|
Thanks everyone. I'll create the new issue to consider relocating the revisions toggle if it hasn't been created yet. For the future: the only way to prevent this kind of layout issues is that the design team must provide design also for the 'Show button text labels' preference. New UIs should not be merged without accounting for 'Show button text labels'. |
|
That's a note for all contributors to the project, it's not useful to single out one group of contributors. |
@jasmussen can you please add more context and clarify? Thanks. |
|
It also seems the "show button text labels" is being applied too broadly to areas where it was not intended to be, like utility actions in panel headers and inspector tabs. |
I kindly disagre. Rather, the "show button text labels" preference should be appliet to all icon buttons and any design should take it into account. |
|
Yeah, I don't think we'll agree on that :) Expanding the application of this preference beyond toolbars is a stretch and would need to be done thoughtfully for each context. Cases like this one cannot be trivially turned into text actions: 
|
Indeed, we don't agree. I'm aiming to make the UI as accessible as possible. I can only note that it seems that's not the focus for everyone in this project.
I would argue that cases like this one (that example is from the data views) are just a less than ideal design that I'd call a 'wrong' design, not to blame anyone. And it's it wrong because it didn't take into consideration the preference since the beginning. I do realize that organizine the complexity of the editor UI is challenging but hiding content, removing or truncating visible text and the like. isn't a good strategy to make the UI usable for everyone. |
…text labels' (WordPress#63243) * Use compact variant for close butotn in complementary area. * Fix global styles header buttons when they show text. * Improve flexbox layout. * Adjust gap. * Set button size to small. * Remove no longer necessary margins. Co-authored-by: afercia <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]>
Temporary fix for #61761
A more solid solution should be implemented by changing the design and not placing buttons in the panels header in the first place. This PR overrides the Button component CSS, which isn't ideal but it is worth noting the 'Show button text labels' feature itself overrides the base component CSS. An alternative solution is being explored in #61763
Additionally, makes the size of the Close button consistent with the size of the other buttons.
What?
The buttons text overlaps.
Why?
Overlapping text isn't readable.
How?
Makes the button width be
autowhen 'Show button text labels' is enabled.Makes the Close button use the
compactsize so that it is consistent with the other buttons.Testing Instructions
window.wp.data.dispatch( 'core/preferences' ).set( 'core', 'showIconLabels', true )window.wp.data.dispatch( 'core/preferences' ).set( 'core', 'showIconLabels', false )Testing Instructions for Keyboard
Screenshots or screencast
Overlapping text - Before: English and italian
After:
Close button size before (all buttons focused to better illustrate):
After: