-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Determine initial inspector tab from block editor state #46907
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useSelect } from '@wordpress/data'; | ||
import { TabPanel } from '@wordpress/components'; | ||
|
||
/** | ||
|
@@ -10,22 +11,18 @@ import { TAB_SETTINGS, TAB_STYLES, TAB_LIST_VIEW } from './utils'; | |
import SettingsTab from './settings-tab'; | ||
import StylesTab from './styles-tab'; | ||
import InspectorControls from '../inspector-controls'; | ||
import useIsListViewTabDisabled from './use-is-list-view-tab-disabled'; | ||
import { store as blockEditorStore } from '../../store'; | ||
|
||
export default function InspectorControlsTabs( { | ||
blockName, | ||
clientId, | ||
hasBlockStyles, | ||
tabs, | ||
} ) { | ||
// The tabs panel will mount before fills are rendered to the list view | ||
// slot. This means the list view tab isn't initially included in the | ||
// available tabs so the panel defaults selection to the styles tab | ||
// which at the time is the first tab. This check allows blocks known to | ||
// include the list view tab to set it as the tab selected by default. | ||
const initialTabName = ! useIsListViewTabDisabled( blockName ) | ||
? TAB_LIST_VIEW.name | ||
: undefined; | ||
const initialTabName = useSelect( ( select ) => { | ||
const { getDefaultInspectorControlsTab } = select( blockEditorStore ); | ||
return getDefaultInspectorControlsTab() ?? TAB_STYLES.name; | ||
}, [] ); | ||
|
||
return ( | ||
<TabPanel | ||
|
@@ -53,7 +50,9 @@ export default function InspectorControlsTabs( { | |
|
||
if ( tab.name === TAB_LIST_VIEW.name ) { | ||
return ( | ||
<InspectorControls.Slot __experimentalGroup="list" /> | ||
<div> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did we add the |
||
<InspectorControls.Slot __experimentalGroup="list" /> | ||
</div> | ||
); | ||
} | ||
} } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,15 +42,13 @@ export default function useInspectorControlsTabs( blockName ) { | |
color: colorGroup, | ||
default: defaultGroup, | ||
dimensions: dimensionsGroup, | ||
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 ) { | ||
Comment on lines
-45
to
-53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't follow these changes. Why did they need to happen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know @aaronrobertshaw worked on some of this so tagging him for additional context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if ( ! listViewDisabled ) { | ||
tabs.push( TAB_LIST_VIEW ); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?