Styles panel: Rearrange the Revisions and Additional CSS toggles #66476
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: +41 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Thank you! I have nothing against the move, and it does decrease the icon overload. I've smoke tested revisions/stylebook in the editor and it works well. Since this affects the UI mainly, I'll lean on the advice of @jameskoster and co, since they have a better idea of how it would fit within any upcoming design changes.
+1 |
|
I think I know, more or less, why the tests are failing on this PR as the 'Revisions' button in the panel header has been removed. However, the Output of the failing test on trunk: |
I'm not opposed to that but then the styling should be consistend for all the similar navigation buttons. I created #66448 to address that. |
|
To avoid multiple changes in quick succession should we wait for a final design in #66376? |
|
Flaky tests detected in f150185. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11609443337
|
@jameskoster as I mentioned in #66307 (comment) I'm not sure I understand why #66376 is a blocker for this PR. Basically, #66376 aims to move the entire Styles panel but it doesn't touch its content. Instead, this PR is about the Styles panel content. |
|
Checking out this PR and I see the new buttons are in a sort of segmented button with all borders on. This kinda keeps the problems around - what are these buttons and why are they so separated and special. In fact the previous way of having a description and a simple text + chevron (no border) is the right choice for this drill down panel. Can we try the suggestion from @t-hamano and @jameskoster - have explanatory label (that always shows) and normal link-to buttons (text and chevron)? |
|
@draganescu thanks for your feedback.
Anyways, I'll gladly change to one of the built-in styles please just tell me which one to use. Please consider the same styling should be used for all the other occurrences detailed on #66448
I'm not sure I understand the point about labels. These buttons do have visible labels. Did you mean the descriptions perhaps? Those must be removed as they are aunique ad-hoc, inconsistent pattern. Tehy're duplicated in the sub-panels and they just add clutter.
I'm not comfortable with this kind of tone. Saying |
|
In the screenshot below: these are the available built-in styles, respectively:
For completeness, there is also an |
afercia
force-pushed
the
fix/relocate-styles-panel-buttons
branch
from
46b623b
to
7643334
Compare
|
Latest commit tries the
|
Thanks for iterating @afercia - I think the style I was reffering to is
I don't think consistency should trump any other objective. The descriptions are there because the buttons "Blocks", "Additional CSS" and "Styles revisions" are disconnected from the above options, although in the same functional space "Styling". For a well versed user they may sound redundant but the role of these descriptions is discovery. And since your PR helps discovery by moving additional css and revisions into the main display then we should also leave the descriptions in place, so at the very least the buttons make sense in this context. Previously, by having them in secondary areas (the dot menu and the extra panel buttons) the intent was to highlight that they're "secondary", special utility. You'd have to know what you want to access them. Here, bringing them front and center we require the descriptions so that users have the benefit of learning what these things do before venturing in. They're deffinitely not clutter. |
I kindly disagree. As I mentioned, that's a unique, inconsistent pattern. I can't support this kind of design direction as I think consistency is key and all that text adds unnecessary cognitive load, so that's also related to usability and accessibility. Furthermore: descriptions should be placed after the object they refer to and never before. Also, descriptions should be programmatically associated to the object they refer to by the means of an About discoverability: these descriptions are used also in the sub-panels. For example, as an usrer, if I want to discover what 'Additional CSS' is I can just click on it and get the panel with the description. Overall, I think the descriptions in the main panel are just redundant and inconsistent and their design is arguable. I vote for removing them. |
To me, as I already mentioned, all the navigation buttons in the styles panel and its sub-pabels should look the same. At the moment, they are largely inconsistent, see #66448 Controls that have the same functionality should use the same styling. This is key to allow users to identify, predict, and understand what these controls are. To me, this point sounds so obvious that it shouldn't even warrant discussing it. I'm not sure the |
|
Just reminding everyone this PR also fixes and existing bug:
That should be fixed, either by this PR or a new one. |
There was a problem hiding this comment.
I'm in favor of this. It also seems like an iteration that shouldn’t much impact further design efforts in #66376 but I could be missing something. One thing I noticed while testing—though it’s probably unrelated to any changes here—is that when drilling down to Revisions or Blocks, the scroll position seems like it should reset to the top. It seems to do that a shorter viewport heights, but I'm seeing it around 680px.
styles-sidebar-drilldown-scroll-position.mp4
|
I'm a little late to this party but appreciate the discussion all around 👍 My vote would be to hold off on this PR until the dust settles on #66376. As I noted over on the original issue, the consistency argument has been raised a few times here and this proposed relocation swaps one inconsistency for another. The new location would still be inconsistent with the Styles panel in the site editor. There is active work happening on the broader issue for the Style Book via #66376. The consistency argument can also be applied to limiting changes to the UI. We can avoid making users adapt to multiple successive UI updates, so in my view it doesn't cost us a lot here to be patient and skip one more small paper cut to users. This is likely part of the reasoning behind @jameskoster's earlier comment. |
|
Fair enough, i will set this PR on hold. If #66376 doesn't make it for the next WordPress release I would suggest to bring back to life this PR and have at least this partial improvement in the release. |
@stokesman interesting. I see that happens (randomly) also when clicking on Typography or Colors. It seems it doesn't happen with Firefox. I'll check whether there's any code that specifically handles scrolling to top otherwise it's probsbly something related to how browsers treat 'scroll into view' of focused element in a different way. |
afercia
force-pushed
the
fix/relocate-styles-panel-buttons
branch
from
7643334
to
f150185
Compare
|
@stokesman I spent some time debugging the scroll to top issue. I can reproduce it also on trunk, by altering the height of the root panel. I removed the two paragraph of the descriptions just to alter the panel height. It appears that when the height of the root panel is shorter, drilling-down into some sub-panels shows that behavior, only with Chrome. As such, it seems something unrelated to this PR.
Animated GIF to illustrate:
|
|
It looks like #66376 has been closed as resolved. Can we proceed with this PR? |
afercia
force-pushed
the
fix/relocate-styles-panel-buttons
branch
from
f150185
to
cf215a5
Compare
|
Rebased. |
|
I tested this PR again. One concern is that the preview button doesn't work when I access the revision screen from the new Styles screen: 37dff762ad8d30bfa6126dfc35cdb18e.mp4As for moving the revisions button, it might be a good idea to think a bit more about the ideal UI in #66719. However, I think other UI improvements (Blocks and Additional CSS) can be pushed forward with this PR. |
Interesting, good catch. Probably not a big issue to fix, would need some investigation. One more important thing to mention is that on current trunk, when removing any custom css from Additional CSS, then there's no longer a way to access Additional CSS again from the new Styles panel. The button disappears. There's no other control to access it. See animated GIF below:
|
|
Thinking that the 'Styles revisions' button should now be shown conditionally;
This will be even more true with the proposed change discussed on #66606 where the 'Last modified' button will be labeled 'Revisions'. As such, the Styles revisions within the Styles panel should only be displayed in the 'edit' canvas mode (i.e. the old styles panel on the right). |
afercia
force-pushed
the
fix/relocate-styles-panel-buttons
branch
from
cf215a5
to
a2de580
Compare
|
Regarding this point:
This is the current behavior but I'm wondering what is the rationale behind it. Why the style book view should be switched back to the normal view when editing Additional CSS? To me, it makes sense to allow users to edit custom CSS also in the Style Book view. For example: to see changes to headings etc. Screenshot:
|
afercia
force-pushed
the
fix/relocate-styles-panel-buttons
branch
from
a2de580
to
cbfc92b
Compare
Fixes #66307
Fixes #66333
What?
The way to access Styles revisions and Additional CSS is inconsistent and little discoverable, especially for the Additional CSS. These features live in sub-panels of the Styles 'drill-down' menu. Other sub-panels use standard navigation buttons with a chevron icon to access the sub-panels.
For more details, please refer to the conversations on #66307
and #66333.
Why?
For consistency and better user experiences, accessing the sub-panels of the Styles drill-down menu should use a consistent pattern. Features should be well discoverable and not buried down into an ellipsis menu.
How?
Additionally: FIxes a bug where accessing the Additional CSS from the navigation button at the bottom of the Styles panel
did not switch back the canvas to the default view when the Style Book was enabled. See #66333 (comment)
Testing Instructions
revisionand titleCustom Stylesfrom the database.Edit:
Testing Instructions for Keyboard
Screenshots or screencast
Before and after: