Components: replace TabPanel with Tabs in the editor Document Overview sidebar
#57082
Conversation
|
Size Change: +166 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 80e6708. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7215568344
|
chad1008
force-pushed
the
tabs-list-view-sidebar
branch
from
bb4ab07 to
80e6708
Compare
chad1008
added
[Type] Enhancement
chad1008
changed the title
TabPanel with Tabs in the editor List ViewTabPanel with Tabs in the editor Document Overview sidebar
|
Thanks for working on this @chad1008, and for the thorough testing instructions!
Some of the interaction flow feels a bit weird to me (for example, not moving focus), but I'll leave that alone, as it's not introduced here. Focus order
For me, the 'before' behaviour, where the close button is before the tab control in the focus order, is more correct than the 'after' behaviour, and we'd ideally change the Editor Settings sidebar behaviour to match. I'm not aware of any specific guidance regarding this particular scenario, where other controls are at play, but in general, I would expect the tab panel to immediately follow the tabs in the focus order, if at all possible...
Tab selectionI also wanted to briefly mention tab selection.
Is there a reason why selection doesn't follow focus here?
I don't want to assume we haven't had the conversation, but it doesn't feel like there's a good reason not to. Not a blocker by any means. |
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code changes look good. I always followed the testing instructions and couldn't spot any regressions 🚀
I'd be more comfortable if @andrewserong also took a look before approving, given his extensive work on the list view in the past months.
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'd be more comfortable if @andrewserong also took a look before approving
Thanks for the ping, and for the work here!
There are some visual changes at the moment regarding hover effects and text alignment on the tabs. These are being discuss/addressed in #57121
I see this is now centering the labels for the buttons. I think these should be left aligned as on trunk unless this is an intentional design decision? To my eyes it looks a bit unbalanced having them be centered as other panels (like the block or template inspector controls) are left aligned.
| Trunk | This PR |
|---|---|
 |
 |
While it's fairly subtle, because the list view is a prominent part of the UI, I think it'd be good to settle on the desired design here before landing the PR.
In terms of usability, the keyboard behaviour is mostly working for me. The main issue I ran into is if you press the Right arrow key to go to Outline and then the Left arrow key to go back to the list view, then the focus behaviour appears to move the focus to the first selected list view item. Is that intentional? It feels like a bug to me, as I believe that focusing behaviour is only intended to run when the list view is initially opened. If it is intentional, it might be worth checking with @alexstine here, too, as I could imagine it being confusing when moving between panels.
2023-12-21.10.31.16.mp4
I'll be AFK after tomorrow, so apologies if I don't get back to this PR quickly! If it's still open in the new year, I'm happy to give it a re-review then 🙂
Good point! #57275 has just been merged, and it contains the style tweaks to change the tabs text alignment :)
That also feels like a bug, we should look more into it.
@chad1008 is also going to be AFK. Let's resume work on this after the holidays |
Sounds good. Happy to help with re-reviews then! |
chad1008
force-pushed
the
tabs-list-view-sidebar
branch
from
365d2e1 to
0d2c78e
Compare
|
And we're back! Thanks for jumping in on this one @andrewserong, very good catch. The reason this is happening is because I enabled Turning
Sorry for overlooking that, and thank you again for spotting it. |
There was a problem hiding this comment.
Thanks for the updates @chad1008! That's resolved the keyboard behaviour for me 👍
In re-testing, unfortunately I think this PR appears to break the windowing logic for the list view, which controls the showing and hiding of list view items on very long posts. On trunk, if I scroll up and down the list view, items and shown and hidden. With this PR applied, the updates aren't occurring:
Trunk
2024-01-03.16.26.47.mp4
This PR
2024-01-03.16.27.11.mp4
For quickly testing very long posts, I'll either use a lorem ipsum generator or copy & paste an old book from the "other" Gutenberg 😄.
I'm not too sure what the cause is, but in case it helps digging into it, the list view calls useFixedWindowList here, and useFixedWindowList is defined here.
I wonder if something about the Tabs.TabPanel could be interfering with the useFixedWindowList's checking for a scroll container? I notice with this PR applied that the line that calls getScrollContainer returns the overall html document instead of the list view container, which is unexpected:
Another potential thing to look into if it isn't Tabs.TabPanel — could any of the CSS changes in this PR affect the getScrollContainer behaviour?
One other thing to keep in mind with changes to this component is that @youknowriad has opened a PR (#57467) to propose moving the component to live within the editor package so that it can be shared between both the post and site editors. So, whichever PR lands first, the other one will likely need some rebasing 🙂
Let me know how you go, happy to help with continued testing!
andrewserong
added
the
[Feature] List View
#57467 got merged, so this PR needs to be rebased. Let's also make sure we keep in mind the differences between post and site editor as highlighted in #57467 (comment) |
@andrewserong , Chad and I debugged the issue and found out that the problem may be caused by Luckily, there seems to be an easy fix — adding the I will let @chad1008 work on implementing the fix (together with rebasing after #57467 got merged), and then hopefully we'll be able to merge this PR 🤞 |
Nice debugging, folks! Let me know when this is ready for another review and I'll give it another spin 🙂 |
chad1008
force-pushed
the
tabs-list-view-sidebar
branch
from
ed81b50 to
3eaaa01
Compare
|
This PR slipped off my radar somehow, but I've made the change mentioned above that should prevent the poorly rendering scroll container for the list view. I've also rebased to incorporate the newest changes from trunk. Would love a fresh opinion whenever you have a moment @andrewserong! |
There was a problem hiding this comment.
Thanks for the ping, this is looking so close now @chad1008! The list view keyboard behaviour all appears to be working nicely for me now, and the issues we'd uncovered so far all appear to have been resolved (can switch between List View and Outline easily via keyboard, windowing logic is working).
While comparing between trunk and this PR, I noticed that it looks like some of the re-ordering means that the document outline has lost its padding. I've left a comment where I think there might be a classname missing for the document outline? This is the set of rules that don't appear to be applying with this PR active:
The result is that the document outline appears flush against the side of the viewport, instead of having a little breathing room (most notable when an h1 tag is in use):
Trunk
This PR
Perhaps a little clearer when navigating through the outline via keyboard, where the left edge of the outline gets cut off:
| Trunk | This PR |
|---|---|
 |
 |
Thanks again for all the detailed work on this one, I know it's a nuanced one!
|
Thanks you as always @andrewserong, both points on your feedback were good catches! There is still a small visual shift in the placement of the second tab, which comes from the spacing for the close button no longer being an implicitly declared margin. I don't think it looks problematic but happy to tweak it if need be. I did also add one small change replacing role based selectors with actual class names, based on some feedback on a different implementation of this component. It shouldn't have any impact on the look of the sidebar. |
There was a problem hiding this comment.
Thanks for the updates @chad1008, the document outline is looking good now!
There is still a small visual shift in the placement of the second tab, which comes from the spacing for the close button no longer being an implicitly declared margin. I don't think it looks problematic but happy to tweak it if need be.
That one looks fine to me, I doubt anyone will really notice it 🙂
In re-testing I ran into one blocking issue which is that the horizontal scrollbar appears to no longer be available in deeply nested lists. This is more common in pretty complex templates, but here's an example with a very heavily nested series of Group blocks:
Trunk
On hover, a horizontal scrollbar is available:
2024-01-29.16.10.45.mp4
This PR
No horizontal scrollbar is available:
2024-01-29.16.11.09.mp4
Other than that, everything else appears to be testing nicely so far! It might be good to give this another rebase, too, so that we can test it in combination with some of the list view drag and drop changes that landed last week. Rebasing locally, it seems to play nicely, though 👍
chad1008
force-pushed
the
tabs-list-view-sidebar
branch
from
3ff66b3 to
3bd148c
Compare
|
thank you yet again @andrewserong for testing so thoroughly and finding the issues i'd missed! Some of the DOM structure was off, but I've addressed that and the scrollbars should be good now! I've also performed a fresh rebase and everything is testing well for me locally, but let me know that you think! |
There was a problem hiding this comment.
Wonderful, this is all testing well for me now @chad1008!
✅ Works well in a short list view
✅ In long posts and pages the windowing logic appears to be working correctly
✅ In nested lists, the horizontal scrollbar is available
✅ Keyboard behaviour when navigating between tabs and in the list view is working well
✅ Short list view expanding into a long list engages windowing logic correctly
✅ Document outline styling is correct and links work as expected
✅ Works appropriately in the site editor, too
LGTM, thanks again for all the back and forth! ✨
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
|
thank you again @andrewserong! This PR was greatly improved by your input. I really appreciate it. |
|
Any time! Likewise, thanks again for all the iterations here! 🙇 |
What?
Replaces the legacy TabPanel component with the new Tabs component.
Why?
Part of the work outlined in #52997
How?
TabPanelis replaced byTabsand its sub-components.The biggest change in terms of actual behavior is the order of the tab stops. Previously, the close button had to be rendered outside of (specifically before)
TabPaneland then placed next to the tabs via CSS. WithTabswe can actually compose the sidebar with the button right where we want it. This changes the flow of focus when tabbing into the sidebar:This matches what the Editor Settings sidebar on the other side of the screen does, but I wanted to confirm this is an acceptable change from an a11y point of view, for both sidebars. cc @andrewhayward @alexstine
Notes
Buttoncomponents by default #57121Testing Instructions
alt+shift+oon Windows, andcontrol+option+oon Mac) to confirm it opens and closes the sidebar properlyThis sidebar has some specific focus behavior built-in. The following tests are to double check we aren't breaking any of that.
tl;dr:
On a post with no blocks:
On a post with blocks:
Testing Instructions for Keyboard
ctrl+`four times to navigate to the top toolbar regionctrl+`to once again navigate to the top toolbar