[WIP] Block Editor Store: Allow management of block inspector tabs #46271
Conversation
|
Size Change: +287 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Thanks for getting the ball rolling here. Some things we thought about connected to this:
|
Basically what we need is to be able to dispatch an action will which select a given tab. That will allow us to force select the |
It might be useful to have one that creates the necessary parts in the store (this one) and one that connects them to the component (the other one!) |
The idea here is we set the default tab in the store via the new actions, that default tab is retrieved and used as the I don't think the move to ariakit for the TabPanel will happen within the timeframe we need for the Navigation block.
The idea with this PR is that we are only storing a default tab not the currently selected tab. It will only be used when first selecting a block, setting the TabPanel's initial tab prop. Outside of that, the default tab, if not specified in the store, would follow the normal rules. i.e. List View > Styles > Settings. Essentially, defaulting to the first tab. I hope that sheds a bit of light on what the current thinking is. Happy to adjust course if there are any issues with it.
I had some in-progress work on leveraging the new config in the store to effect defaulting the Navigation Link block to the settings tab along the lines proposed above. I also have a second follow-up that allows plugins to enable/disable tabs. The plan is to have those published in the next couple of days. |
|
ariakit supports setting the initial tab as part of the |
| * | ||
| * @return {Object} Updated state. | ||
| */ | ||
| export function blockInspectorTabs( state = {}, action ) { |
There was a problem hiding this comment.
It's an interesting one, because reducers are usually used for values that are likely to change at runtime. I wonder if there will be a use case for changing these values while the editor is running.
My feeling is that these values will probably be set as an initial configuration of the editor, which is often the job of the block editor settings (although technically they can change at runtime too, but often don't).
I think the only pain point will be that it's a little more difficult to handle deeply nested objects, but I think the data structure could be simplified (as mentioned in my other comment).
Editor settings can also be set in PHP, so that another advantage.
There was a problem hiding this comment.
Just to note we have a use case for the Nav offcanvas experiment whereby we'd need to programmatically select the default tab as a result of an interaction with the "edit" button.
There was a problem hiding this comment.
Appreciate the feedback 👍
because reducers are usually used for values that are likely to change at runtime
The desire to set the default tab for the Navigation Link block, depending on if it was edited via the parent Navigation block's list view, drove the need for these values to change at runtime.
My feeling is that these values will probably be set as an initial configuration of the editor, which is often the job of the block editor settings
Splitting the management of the block inspector tab display so the default tabs are handled separately would open up the possibility of using the block editor settings. I'll be happy to explore that approach as well.
In the meantime, I wonder if this is reason enough to reconsider disabling tabs entirely for the Navigation Link block in the short term. It would allow us to avoid adding anything to the store related to the tabs or navigation experiments and all tab management settings would still be contained in a single location.
There was a problem hiding this comment.
Just to note we have a use case for the Nav offcanvas experiment whereby we'd need to programmatically select the default tab as a result of an interaction with the "edit" button.
@getdave Aaron explained this to me in a private message. My mistake for not reading the PR description thoroughly.
The main problem is that I don't think a stable API should be introduced for two experimental features. There is a proposal for experimental selectors/actions, which would help:
I don't know if the __experimental prefix is still allowed? cc @adamziel
Alternatively the prefix could be used, but the selectors/actions would need to be stabilized or removed for alternatives before 6.2, so that they're not released with WordPress core.
There was a problem hiding this comment.
In the meantime, I wonder if this is reason enough to reconsider disabling tabs entirely for the Navigation Link block in the short term. It would allow us to avoid adding anything to the store related to the tabs or navigation experiments and all tab management settings would still be contained in a single location.
@scruffian If we go down this route then will this compromise the user testing of the Nav offcanvas experiment?
There was a problem hiding this comment.
I think as long as we can put the link settings above the color ones then this should be an ok interim solution.
There was a problem hiding this comment.
Here is what the navigation link block currently looks like with the tabs disabled. The label input is the first much like I imagine is desired.
There was a problem hiding this comment.
I don't know if the __experimental prefix is still allowed? cc @adamziel
Once the tooling is merged it will be safe to stop introducing new prefixed APIs entirely, but we're not there yet. Ideally there wouldn't be anything __experimental in public exports, but at the moment we cant avoid it sometimes.
| case 'SET_DEFAULT_BLOCK_INSPECTOR_TAB': | ||
| return { | ||
| ...state, | ||
| [ action.blockName ]: { | ||
| ...state?.[ action.blockName ], | ||
| defaultTab: action.defaultTab, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The default tab could be a separate reducer, and it'd simplify the code quite a bit. DISABLE_BLOCK_INSPECTOR_TABS could be simplified to adding a block name to a unique array (so that the state is something like this disabledInspectorTabs: [ 'core/freeform', ... ]).
SET_DEFAULT_BLOCK_INSPECTOR_TAB could be storing a map of block names to tab names (defaultInspectorTabs: { 'core/navigation': 'menu', ... }).
Or you could use combineReducers to have nested state (e.g. with this shape - inspectorTabs: { disabled: [ 'core/freeform' ], defaultTab: { 'core/navigation': 'menu' } }
There was a problem hiding this comment.
Thanks for highlighting this.
DISABLE_BLOCK_INSPECTOR_TABS could be simplified to adding a block name to a unique array (so that the state is something like this disabledInspectorTabs: [ 'core/freeform', ... ]).
I think we'll need to tweak the proposed structure a little further to allow for overriding the default display of tabs for all blocks.
There was a problem hiding this comment.
Is an all block setting definitely needed?
I do wonder if that could be explored as a user preference (especially as an accessibility option), but worth taking this one challenge at a time.
There was a problem hiding this comment.
During some recent discussions, it was requested that plugins be able to disable/enable tabs across all blocks but that it shouldn't necessarily be something controlled by end users.
I'll have another think about it. It could also be something we expanded upon in the future.
|
Let's leave this PR open but to simplify let's pivot and try out an alternative whereby we use block editor settings to control the tabs. |
|
A PR trialing using the block editor settings to manage tab display and disable tabs for the Navigation Link block is available in: #46321 |
|
Closed in favour of #46321 |
🚧 🚧 WIP 🚧 🚧
Related:
What?
Provides a means of overriding the display of block inspector tabs on a per-block basis. Also, allows for configuring default tabs for different blocks e.g. making the settings tab the default when a navigation link is edited via the navigation block's list view tab.
Why?
How?
Adds an API to the block editor store to allow management of block inspector tabs.
Testing Instructions
TBA.
Testing Instructions for Keyboard
TBA.
Screenshots or screencast
TBA.