Skip to content
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

View Page and Toggle Zoom Out buttons should have width auto when showIconLabels is true #65491

Closed
2 tasks done
afercia opened this issue Sep 19, 2024 · 8 comments · Fixed by #66038
Closed
2 tasks done
Assignees
Labels
[Feature] Show button text labels A preference in the Post and Site Editor that makes buttons show text instead of icons [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Sep 19, 2024

Description

See #65183
See #65311
See #65404

When enabling the 'Show button text labels' preference, most buttons in the top bar and block toolbars should have a width auto to show the text labels in a single line. View Page and Toggle Zoom Out don't. Screenshot:

Screenshot 2024-09-19 at 15 51 53

It would be great to have a more reliable mechanism to set the width auto to icon buttons that reveal their text when showIconLabels is true.

Also, it would be great that any new design and UI change is designed taking into account showIconLabels and tested for it. Cc @WordPress/gutenberg-design

The 'Zoom out` feature is now not flagged behind an experiment any longer so this appeear something to fix soon.

Additionally, I'd like to remind everyone that the verb 'Toggle' must be avoided as it's not well translatable in many languages.
See #42492
See https://core.trac.wordpress.org/ticket/34753

Cc @WordPress/gutenberg-core

Step-by-step reproduction instructions

  • Go to the Site editor.
  • Edit a published Page.
  • Enable the 'Show button text labels' preference.
  • Observe the View Page and Toggle Zoom Out buttons text wraps in two lines.

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
@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site labels Sep 19, 2024
@afercia afercia moved this to 📥 Todo in WordPress 6.7 Editor Tasks Sep 19, 2024
@afercia
Copy link
Contributor Author

afercia commented Sep 19, 2024

One more issue with the 'Toggoel Zoom Out' is that buttons should not show a tooltip with the same text of the visible text. In fact, for most buttons the tooltip is conditional and doesn't show up when the label is visible.

Screenshots; In the Site Editor and Post Editor, the tooltip just repeats the visible text:

Post editor:

Screenshot 2024-09-19 at 16 13 44

Site editor:

Screenshot 2024-09-19 at 16 14 12

@afercia
Copy link
Contributor Author

afercia commented Sep 24, 2024

Now that #65573 has been merged, I submitted a quick PR to remove the verb Toggle,

Besides making the string better translatable, shortening the label helps not breaking the layout when the 'Show button text labels' preference is enabled. Screenshot with the WP admin language set to English:

Screenshot 2024-09-24 at 13 33 48

However, other languages provide longer strings so the layout isn'g great yet. I stand corrected with my previous description as it seems the widht auto is set correctly on this buttons when showIconLabels is true. It appears it's the grid layout of the container that doesn't take into account longer strings so it's a broader issue that should be addressed on the CSS side.

Screenshots in other languages:

Screenshot 2024-09-24 at 13 35 17

Screenshot 2024-09-24 at 13 36 02

@aaronrobertshaw
Copy link
Contributor

I stand corrected with my previous description as it seems the widht auto is set correctly on this buttons when showIconLabels is true.

I can confirm the buttons in the editor header have width: auto already applied when showIconLabels is set to true.

Image

It appears it's the grid layout of the container that doesn't take into account longer strings so it's a broader issue that should be addressed on the CSS side.

This looks to be because the buttons in question don't enforce white-space: nowrap.

Buttons for pinned plugin sidebars are made tertiary buttons which come with white-space: nowrap set. They also get some padding that buttons like Zoom out and View don't. Maybe we could get some design direction on exactly how we want all the buttons here to end up.

Image

The possibility of an unknown number of pinned plugins could make a completely clean solution tricky. For now though creating a bunch of test puglin sidebars, it just reduces the available space for the center document bar.

The document tools on the left of the editor header mostly get hidden at smaller viewports, so do not seem as susceptible to wrapping.

I drafted a quick PR that simply adds white-space: nowrap for buttons in the editor header when showIconLabels is true. It seems to do the job.

While testing this, I noticed the buttons in the Global Styles sidebar's header are also wrapping and overlapping when showIconLabels is true. The available real estate there is unlikely to change and I have some vague recollections around components in the editor needing to remain unaware of the showIconLabels preference. That could prevent doing something like; if the preference is enabled, move the Style Book, and revisions buttons into the more menu there. Figured it was worth raising in case others have ideas while looking at this issue.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 11, 2024
@afercia
Copy link
Contributor Author

afercia commented Oct 11, 2024

@aaronrobertshaw thanks for looking into this.

Personally, I think the only way to improve showIconLabels is that any new design should provide a design for this preference too. The current state of the UI when showIconLabels is true is honestly a poor state. Designing for it should be an integral part of the design phase.

This looks to be because the buttons in question don't enforce white-space: nowrap

Yes that's one of the issues. It would be interesting to understand why the default button doesn't use white-space: nowrap while the primary, secondary, and tertiary variants do. I do understand the link variant should allow text to wrap, but why the default does? GIF to illustrate:
Cc @WordPress/gutenberg-components

Image

The possibility of an unknown number of pinned plugins could make a completely clean solution tricky. For now though creating a bunch of test puglin sidebars, it just reduces the available space for the center document bar.

Regarding, the document bar, I'm proposing to entirely rethink it. Its implementation is less than ideal from an usability and accessibility perspective and it is a problem when the viewport is small. I don't think hiding controls and content is the best approach to responsive design in the first place. See #65238

While testing this, I noticed the buttons in the Global Styles sidebar's header are also wrapping and overlapping when showIconLabels is true. The available real estate there is unlikely to change and I have some vague recollections around components in the editor needing to remain unaware of the showIconLabels preference.

That is a known issue and it's reported on #61761
I don't think introducing exceptions to showIconLabels would be a good option. Rather, there is a broader issue with all the panels headers. For better usability and accessibility, all panels should have a title. That was first tried on #61553. From there, I created a new issue #63251 where I'm proposing to add an optional sub-header to all panels. The sub-header may contain the buttons when showIconLabels is true. That proposal was inspired by the solution implemented for the LinkControl where the buttons go in a new line when showIconLabels is true, see #61726

@ciampo
Copy link
Contributor

ciampo commented Oct 11, 2024

@afercia moved the text wrapping conversation to a separate issue: #66049

I have some vague recollections around components in the editor needing to remain unaware of the showIconLabels preference

@aaronrobertshaw yeah, basically code in @wordpress/components shouldn't rely on WordPress-specific settings. We could think about adding a more generic functionality to Button that is not tied to the settings — and then the consumer of the Button component would be in change of "connecting" the WordPress setting to the hypothetical Button prop

@getdave
Copy link
Contributor

getdave commented Oct 11, 2024

👋 Thanks for all the discussion and work on this.

Looking at this with a 6.7 lense for a second, I could be wrong but it seems like there are a lot of moving parts and fixes being proposed that could expand in scope.

Would it be better to defer this work until 6.8 cycle to allow a more comprehensive solution to be devised as a components level?

I definitely support the effort to improve the current situation but if the complexity is high it's probably not a good fit for this late in the release cycle.

What do you think? Is it possible to "de scope" this to a simple fix or is it a larger problem that could do with more time to resolve?

Update: I just noticed #66038 (comment) is a proposal for a quick fix. So the above might be a moot point.

@afercia
Copy link
Contributor Author

afercia commented Oct 11, 2024

@getdave Personally I'd vote for fixing #66038 for 6.7 and look at the broader issue for 6.8.

@afercia
Copy link
Contributor Author

afercia commented Oct 11, 2024

Would it be better to defer this work until 6.8 cycle to allow a more comprehensive solution to be devised as a components level?

Reminder: to address in a more holistically way the implementation of showIconLabels I created #61763. That is five months ago, already. I also proposed a potential approach in #61824. While that may not be the best approach, I'd appreciate more focus on that issue and any alternative option. Thanks.

@github-project-automation github-project-automation bot moved this from 🏗️ In Progress to ✅ Done in WordPress 6.7 Editor Tasks Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Show button text labels A preference in the Post and Site Editor that makes buttons show text instead of icons [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Edit Site /packages/edit-site [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants