Focus settings tab when editing nav item in Offcanvas experiment list view #46275
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will not be committed and is for illustrative purposes only. We need TabPanel to expose an API for setting the tab.,
getdave
requested review from
draganescu,
scruffian,
MaggieCabrera,
ajlende and
aaronrobertshaw
getdave
commented
Dec 2, 2022
|
|
||
| return ( | ||
| <TabPanel | ||
| className="block-editor-block-inspector__tabs" | ||
| tabs={ tabs } | ||
| initialTabName={ initialTabName } | ||
| key={ clientId } | ||
| onSelect={ setSelectedTab } |
There was a problem hiding this comment.
We must reflect manual tab selection in the editor state.
getdave
commented
Dec 2, 2022
| @@ -1878,4 +1885,5 @@ export default combineReducers( { | |||
| lastBlockInserted, | |||
| temporarilyEditingAsBlocks, | |||
| blockVisibility, | |||
| inspectorControlsTabs, | |||
There was a problem hiding this comment.
Question: should this live in @wordpress/interface or somewhere else?
getdave
commented
Dec 2, 2022
| @@ -1855,6 +1855,13 @@ export function temporarilyEditingAsBlocks( state = '', action ) { | |||
| return state; | |||
| } | |||
|
|
|||
| export function inspectorControlsTabs( state = '', action ) { | |||
| if ( action.type === 'SET_INSPECTOR_CONTROLS_TAB_SELECTED_TAB' ) { | |||
There was a problem hiding this comment.
Question: should this be handled on a per-block basis?
getdave
commented
Dec 2, 2022
| @@ -106,7 +106,7 @@ export function TabPanel( { | |||
| const selectedId = `${ instanceId }-${ selectedTab?.name ?? 'none' }`; | |||
|
|
|||
| useEffect( () => { | |||
| if ( ! selectedTab?.name && tabs.length > 0 ) { | |||
There was a problem hiding this comment.
The TabPanel is an uncontrolled component. initialTabName prop is only for setting the initial tab and from then on the component manages its own state internally.
However, we now need a way to set the internal state from the consumer. There are various ways of achieving such a thing but I will reach out to the folks working on @wordpress/components for advice.
Likely this will be pulled out into a separate PR.
There was a problem hiding this comment.
One option is I guess to make selectedTab optional. If provided the component is expected to be "controlled" and you have to handle the selectedTab state as the consumer.
There was a problem hiding this comment.
Hey Dave, thank you for the ping!
TabPanel has been receiving a high amount of feedback / interest in the past weeks, after months of quiet — that’s quite interesting! We’ve been added support for manual tab activation as an urgent request, but the mid-term plan is to convert the component to internally using ariakit . Last time this was discussed, @talldan showed his interest in working on this refactor.
The reason I’m bringing this up is mainly to give context and to help to coordinate with any ongoing work.
Now, specifically to TabPanel — if I understand correctly, the main ask it to allow TabPanel to be both an uncontrolled and a controlled component. I don't see that as being particularly problematic, but here are my initial considerations:
- the most important aspect would be the API design. Normally a component to expose
value,onChangeand (optionally)defaultValue(for the initial value before it goes uncontrolled). In the case ofTabPanel, it looks likeonSelectalready fulfils the role ofonChange, and thatinitialTabNamefulfils the role of thedefaultValueprop. And therefore, it would be a matter of exposing the correspondingvalueprop (it could be calledtabName, maybe?) and implementing the controlled/uncontrolled logic (we have a handyuseControlledValuehook that could be of help) - we'd need to make sure that these changes are compatible with the planned refactor to
ariakitand with the intention of splitting the component into more modular sub-component - we'd need to make sure that these changes won't affect current usages of
TabPanelin uncontrolled mode (adding more unit tests would be a great first step here)
These were my initial impressions — unfortunately I'm not going to be around during the next weeks (will be back fully in the new year), and therefore I'm going to ping other folks who are familiar with the package to continue this conversation (@mirka @chad1008 @talldan @aaronrobertshaw @andrewserong @tellthemachines )
getdave
commented
Dec 2, 2022
Comment on lines
-28
to
+44
| : undefined; | ||
| : selectedTab; |
There was a problem hiding this comment.
Note to self: we must ensure the default return value of the reducer is also undefined. This will ensure the default behaviour still applies when the state is not set in the editor store.
|
Size Change: +172 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Thanks for exploring this @getdave 👍 I haven't had a chance to dive too deep into this PR yet but wanted to note there's some overlap in this PR with what I'd started in #46271. The intent in #46271 is to not only provide a means of overriding tab display across blocks and per-block-type but also facilitate setting a default tab for individual blocks. Hopefully, without the need to update the TabPanel component as the block inspector tabs were keyed off I planned to sort out the updates to the inspector control tabs, and the navigation link editing, in separate PRs on Monday as I ran out of time today. Let me know if you'd like me to shelve any of the work I was doing on this. |
|
Some of the groundwork for this is already done here: #46271 |
No that's good news. We didn't realise this was underway but it was good to get under the hood here. Essentially our requirement is a means to dispatch an action that will force select one of the tabs in the inspector view for a given block. Happy to help with reviews...etc when you are ready. I suggest we continue in your PR as mine is just a draft/experiment. |
|
Sounds like a plan 👍
The approach I'd formulated with @glendaviesnz was that we might not need to force a tab selection after the block inspector tabs have already been rendered. The TabPanel used is keyed off the current We can work through any kinks over on #46271. |
|
Closing in favour of #46271 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Experiment to try allowing Offcanvas List View to select the
Settingstab in the inspector controls for the given block when you click theEditin the list view.TabPanelto expose a suitable API for selecting tabs.Closes #45951
Why?
#45951
How?
initialTabprop to set the internal state of the select tab in theTabPanelcomponent.settingstab from OffcanvasEditbutton.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast