Replace OffCanvasEditor in browse mode with the List View component #50287
Conversation
scruffian
added
the
[Feature] List View
|
Size Change: +244 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3060636. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4927999458
|
There was a problem hiding this comment.
Clicking page links takes me to that page in the site editor, that seems to be working well.
Expanding sub pages seems to be an issue though. I have Chrome on the right and Safari on the left. Chrome wasn't letting me expand sub pages, but it eventually worked but they collapse immediately. I did the same thing in Safari and it crashed.
Safari seems to let you expand, but the top level items then disappear.
Interestingly in trunk these subpages don't even show. Should we just disable that for now and fix in a followup? |
|
I should have mentioned that, I figured it was just a benefit of the new component. Makes sense to disable to get this one over the line, then follow up. |
|
I fixed an issue with the icon displaying differently, but still looking into the expand behaviour... |
| @@ -264,6 +270,7 @@ export default forwardRef( ( props, ref ) => { | |||
| showAppender={ false } | |||
| blockSettingsMenu={ BlockSettingsDropdown } | |||
| rootClientId={ null } | |||
| onSelect={ null } | |||
There was a problem hiding this comment.
Can't this new prop have a null default instead of having to put it in all list view instances?
There was a problem hiding this comment.
Does it need to be null at all, or can it just be the default of undefined? Or is it better to flag it as null so that it's explicit that it's not a required prop?
There was a problem hiding this comment.
The reason for setting it as null here is that it prevents people from using it in the ListView component, thus keeping the new props private.
There was a problem hiding this comment.
Oh, it's a reset to null after { ...props } I see.
There was a problem hiding this comment.
Is it not a bit confusing that when using the PrivateListView named export the onSelect prop will work as expected, but when using the default export onSelect won't work? (Same thing for the other props that get overridden in the default export).
Should we at least document the default export making it clear that showAppender, blockSettingsMenu, rootClientId and onSelect are not meant to be passed to that component?
|
In my testing in FF there is some CSS bug that hides the parent pages: hiding-pages.mp4 |
andrewserong
added
the
[Type] Code Quality
I believe that's this CSS rule, which might now need to be overridden deliberately for the dark mode version of the list view?
It looks like the sidebar navigation screen is expecting to override rules via components classnames e.g.
But I'm wondering how this should ideally work. @ciampo mentioned in a list view PR recently (#48461 (comment)) that it's better not to try to override via those internal classnames, so I'm curious whether the list view should explicitly allow a dark mode, and have rules specifically for it within the list view component, or if we need to use some CSS variables in the list view styles, and then in the navigation screen, we reset those variables for the desired colors? |
scruffian
force-pushed
the
remove/offcanvaseditor-in-browse-mode
branch
from
64db41f to
02d9556
Compare
I think in this case a simple solution should be sufficient, see #50434. |
|
I thought about this some more and I realised that the @talldan I'm curious what you think about this idea? By providing a I've added a commit to show how it would work anyway. To test it, open browse mode and attempt to edit a custom link. You should see something like this:
|
It's more generic, so definitely much better than building the LinkUI directly into ListView. 👍 Is it possible to break that change out into a separate PR? I think extra private props is ok. Once things have settled down a bit it should be easier to take stock of the props and see if there are any to stabilize and any that need more time. |
|
I like the render extra block UI idea too, not sure if it's perfect, but it's closer to my understanding that the block leaf of the list view should effectively render the edit of the block in a special case, so the edit can decide if it has some behavior for the situation when it's just a leaf on a tree. Or maybe the block leaf of the list view could have some slots which the block's edit that exists in the canvas can fill (for buttons and extra UI)? |
Thank you for the ping! For components inside the Once that's ready, those style overrides should not be necessary anymore! In the meantime, I think it's ok to do style overrides, with a few precautions:
|
Done here: #50465 |
Is this still happening to you? I can't reproduce. I do see a white container with the name of the subpages on safari flickering before we click on the parent page though: 
|
scruffian
force-pushed
the
remove/offcanvaseditor-in-browse-mode
branch
from
9685275 to
363b376
Compare
Add context to the block icon in list view update comment add another prop to list view change fixed color renamed blockSettingsMenu prop
scruffian
force-pushed
the
remove/offcanvaseditor-in-browse-mode
branch
from
363b376 to
3060636
Compare
There was a problem hiding this comment.
This PR is working as intended. I tested it against what's happening on trunk and I find that the behavior is the same plus we have the added extras of having the submenus be collapsible. This uncovered a bug that is already happening on trunk and it's the cause of this I was mentioning before:
Is this still happening to you? I can't reproduce. I do see a white container with the name of the subpages on safari flickering before we click on the parent page though
When we click on a menu item that has submenus on it, we get this. Right is trunk, left is this PR (I had to scroll down to see the whole container):
As this is not being introduced by this PR I'm merely noting here so we can work on it on a separate fix.
I think we can go ahead with this one and continue improving the browse mode experience on further PRs.
What?
Replaces the OffCanvasEditor in the browse mode sidebar with the List View component.
Why?
This is necessary as the OffCanvasEditor component is a duplicate of List View. See #50149.
How?
onSelectprop to the List View and make it privateTesting Instructions
Testing Instructions for Keyboard
As above
Note
This isn't working for custom links. In trunk custom links can be edited inline like this:
I'm not sure how we should handle this as we probably don't want to add a
renderAdditionalBlockUIprop to the list view.