-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Show template center UI when no block is selected #56217
Conversation
…emplate post editor The central template ui to go back to the post was not showing after selecting a block in the canvas. We should only be collapsing this area when we're: - editing a template - top toolbar mode is on - a block is selected - a user has clicked to collapse the area - the viewport is large
Size Change: +935 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
@jeryj thanks for looking into this! Regarding this part:
How do you unselect a block? I'm clicking in the empty space after the footer but for me the central area with the Back button stays hidden, even if no block is selected. Screenshot: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! The center is now always visible when not in top toolbar mode, but as @afercia noticed it still doesn't show when in top toolbar mode and no block is selected. Adding a check to see if a block is selected around line 155 could perhaps fix that?
Another thing I noticed is the tooltip for "Show block tools" is showing when text labels are enabled, though that can probably be fixed separately:
Also the tiny niggling detail of the focus ring for "Show block tools" being partially hidden by the center area 😅
Thanks for noticing this. I'd think this should really be automated in the component, as in: when there is visible text and the same text is going to be used for the tooltip, don't show the tooltip. I suggested this behavior to @ciampo but we didn't came to an agreement. The point is this keeps to be missed because we often forget to test the 'show text labels' preference. Automating the behavior would certainly help. |
Comparing
Also, if there was ever a component where this feature should be implemented, I wonder if |
Independently of which specific components are aware of the feature, I do think this functionality should be dealt with in the components library, because it's an essential part of ensuring accessibility, and it needs to behave consistently across the whole interface. The current implementation of the text labels preference depends on a classname output on the body element, which means Button component doesn't change in itself when its label changes from icon to text: the change is done via CSS only. Adjustments such as removing the tooltip need to be passed to Button conditionally depending on the preference setting, but that requires every single component that renders a Button to know about the preference, and every single developer using a Button to be aware of it. It's the last part that is most concerning, because it requires extensive education and increases the overhead for core contributors and extenders alike. The point of a components library is to provide tools that decrease the mental load and decision-making of developers, and make it easier to build a consistent interface. The other advantage to making the components library responsible for handling the button text labels preference is that component developers will have to take this into account for all new components, avoiding situations where the design of components makes it impossible to implement accessible labels after the fact. See #46299 for an idea of the difficulties with implementing text labels for the block inspector tools. The main obstacle to this is that preferences are editor-specific, and components shouldn't be aware of the environment they're rendering in. One option would be to try a similar approach to the Interface package, where the preferences store is called with a scope that has to be given by the component rendering it. But having to pass in a scope for each component used isn't practical; components are often used by other generic packages such as block-editor. It would be nice if there were a way of detecting what scope/preferences apply in the current context and have components render text/icons depending on the preference set. I'm unsure if there's been any thinking around this already? Cc @talldan who worked on preferences quite a bit. I imagine a similar mechanism would be necessary for implementing dark mode in components too, if that is open to consideration (dark mode across the WP interface would be much welcome accessibility enhancement!) |
I'd second what @tellthemachines said, as in the components library should be responsible, in some way, for handling the button text labels preference.
@ciampo I do understand your arguments. But, the point is there have been several cases where specific implementations of Button didn't take into account the 'show text labels' preference. This keeps to be missed by developers and broke several times. I'd think we need a pragmatical solution to an actual problem. |
Added the |
Flaky tests detected in e901b2a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7105533583
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, this is working very well now!
Thank you @tellthemachines for sharing your perspective!
As you also mentioned, this is the main problem. The components package shouldn't be aware of any editor preferences.
I also agree that this is not great devEx and can lead to poor results.
We've been actually working on this, and the solution that we're landing on is a theme provider. We initially experimented with a So, maybe, the theme could be the means through which we can expose the editor's preference to the components in the package.
Regarding this specific requirement, I don't think that we can reliably find a way to implement this logic. Implementing such a check is not trivial, prone to failing over many edge cases, a cause for inflated bundle size and extra runtime costs, similarly to what discussed in #56231 (reply in thread) |
@ciampo thanks for your feedback. I do understand your concerns but I'd kindly note that you are not providing an alternative solution. We do have a problem. We need to solve it. The 'show text labels' preference broke several times in the past and that's going to happen again, as apparently contributors to this project aren't well aware of it and aren't educated to check for it. Saying no to a specific proposed approach is perfectly fine but blocking the solution of a problem isn't ideal. How can we solve this problem? Thanks. |
I am highlighting a potential high-level way forward for how to "connect" components to the "show text label" preferences, inspired by @tellthemachines 's suggestion :
What I don't have a (good) solution for is the request to automatically detect when the contents of a tooltip popup match the contents of the tooltip trigger, which could have been a more generic, component-centric approach to the "show text labels" problem. More in general, we can't be expected to always have good solutions to every problem. Rushing to implement a sub-par solution can be as harmful (if not more harmful) than the problem we need to fix. And the best way forward, in my opinion, is to foster a culture of collaboration, where better ideas can emerge as a result. |
Fixes #56194
What?
When using the Template Editor from within a post, the central template ui to go back to the post was not showing after selecting a block in the canvas. We should only be collapsing this area when we're:
Why?
Fixes a bug where the central template ui was hidden.
How?
Check for the correct variables before hiding the UI:
Testing Instructions
(Partially stealing these from @afercia's issue - thanks for the clear screenshots!)
Testing Instructions for Keyboard
Screenshots or screencast