Determine initial inspector tab from block editor state #46907
Conversation
draganescu
added
[Type] Enhancement
draganescu
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +188 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
| @@ -477,6 +477,10 @@ _Returns_ | |||
|
|
|||
| - `Array`: ids of top-level and descendant blocks. | |||
|
|
|||
| ### getDefaultInspectorControlsTab | |||
There was a problem hiding this comment.
As it's likely that these will be superceeded by a dedicated package or mechanic for interacting with the UI (as suggested by @talldan) should we mark as unstable?
| @@ -53,7 +50,9 @@ export default function InspectorControlsTabs( { | |||
|
|
|||
| if ( tab.name === TAB_LIST_VIEW.name ) { | |||
| return ( | |||
| <InspectorControls.Slot __experimentalGroup="list" /> | |||
| <div> | |||
| list: listGroup, | ||
| typography: typographyGroup, | ||
| } = InspectorControlsGroups; | ||
|
|
||
| // List View Tab: If there are any fills for the list group add that tab. | ||
| const listViewDisabled = useIsListViewTabDisabled( blockName ); | ||
| const listFills = useSlotFills( listGroup.Slot.__unstableName ); | ||
|
|
||
| if ( ! listViewDisabled && !! listFills && listFills.length ) { |
There was a problem hiding this comment.
I don't follow these changes. Why did they need to happen?
There was a problem hiding this comment.
The check && !! listFills && listFills.length made the list view tab unavailable untill the fills loaded. I have no idea what this achieved, but removing this check allows the state to be 'list` and to set the default.
With the check in place the tabs reverted to 'styles' despite the state for initial tab was 'list' and that happened because of waiting for fills.
The other line is just removed because only the check used that const.
There was a problem hiding this comment.
I know @aaronrobertshaw worked on some of this so tagging him for additional context.
There was a problem hiding this comment.
From memory, the check was in place due to an earlier requirement that tabs were only to be rendered if they had fills. Given the list view tab is a special case using an allow list, it looks superfluous to me as well.
|
This is working well for me. I also tested the navigation link and submenu blocks by removing these lines: |
|
I am not in favor of this change. I believe it is more important for users to always know what will be selected vs. saving a click for visual simplicity. The problem is not now, the problem is at scale. The more blocks that start having different patterns of tabs, the worse the UX becomes for non-sighted keyboard users. Having a solid default tab ensures users know what to expect vs. some conditional devs wrote in to the code. |
|
I have some qualms about using redux state for this purpose. There's the potential for bugs if the state isn't cleared correctly and lingers. There aren't many good alternatives. I suggested an API, but that has had no feedback. Please do leave feedback if you're experiencing the pain points described there.
From watching the video it doesn't seem right when the spacer block is selected and the second tab is active by default. Is there a good reason for that happening? I do understand the reasoning for the Possibly the icon and label for the pencil button could be updated to indicate that it specifically edits the block settings. The visual label is currently quite wordy and could be simplified. |
There was a problem hiding this comment.
I have some qualms about using redux state for this purpose. There's the potential for bugs if the state isn't cleared correctly and lingers.
After writing this I did some testing, and I am indeed seeing this bug. Steps to repro:
- Create a Navigation Block with a Page List block within it.
- Use the 'Edit Page List block' pencil icon from the off canvas editor
- Observe the settings tab is active by default
- Deselect all blocks
- Select the Navigation Block
- Observe the settings tab is still incorrectly the default
|
Ah true bug @talldan I should have seen it even before testing 🤦🏻
The idea is that we think users editing a navigation item via the tree are more interested in settings rather than style. If @SaxonF / @jasmussen are OK we can give up this behavior and just let the default tab be the styles tab. The other problem is that we'd like the navigation block to always default to the list view, particularly when we use the back button in the inspector. This is in line with @alexstine 's point - to always have the same default panel open, it's styles for blocks but it's list view for blocks that have list view. I can either fix the clearing of the state in the tabs component or I can focus on making the navigation block always use the list view. I mean, it's not like the hard-coded block exception in the tabs component is superior, but at least it seems to be a better temporary situation until we get a nice UI management API (#46347). |
I can reproduce the bug as well via slightly different steps. You can skip deselecting all blocks and simply switch to another block on the page that should default to styles e.g. a paragraph. A very similar approach was trialled in #46271 but ultimately abandoned. It might provide additional context and points to consider. At the time it was closed, the biggest issues were it:
|
The Nav block currently defaults to the list view tab on trunk. That said, the code could be cleaned up a little following a subset of the changes in this PR. |
|
One of the key parts of the flow is when editing Menu items (links), which seems mainly addressed, since that doesn't have any sub-tabs. |
It would be convenient if it could remember which tab you were in when you switch between blocks that have the same tabs. Currently when I select a block, switch to settings tab, change a setting, then switch to another block of the same type to change the same setting, I'm back in the Styles tab. But typically I tend to need to change the same setting on multiple blocks, for example, changing 3 similar buttons from size "Small" to "Large". |
|
I'll close this PR as it is not a good approach, but the problem still needs to be resolved. |
|
Sorry for unintentionally deleting the branch. Clicked on the wrong thing 🤦♂️ |
What?
Closes #45951
Why?
Having style as the default works for most cases but for navigation editing the inspector tabs that we expect to be most used are list and settings. So this PR aims to fix defaulting to styles when the off canvas editor is used.
How?
Adds a new state
defaultInspectorControlsTab, selector, action and reducer to the block editor store.Uses this state to decide what is the initial tab when the tab panel is mounted.
Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
inspector-tab-initial.mp4