Skip to content
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.

Sign up for GitHub

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

Inspector: Use the TabbedSidebar component #63184

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Inspector: Use the TabbedSidebar component #63184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24b68c6
Inspector: Use the TabbedSidebar component
scruffian Jul 5, 2024
0621a5b
remove tabs component
scruffian Jul 5, 2024
3d5f602
remove context
scruffian Jul 5, 2024
2d23a17
remove sidebarheader file
scruffian Jul 5, 2024
0963b72
remove new prop
scruffian Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/components/inserter/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ function InserterMenu(
>
<div className="block-editor-inserter__main-area">
<TabbedSidebar
className="block-editor__tabbed-sidebar"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this so that we can set a width for only the inserter a the list view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ends up compiling to block-editor-tabbed-sidebar block-editor__tabbed-sidebar which feels like a workaround. How about one of these options:

  • Give it a name that describes the 350 width, like: block-editor-tabbed-sidebar--wide
  • Name it as the inserter and list view, i.e. block-editor-inserter__tabbed-inserter
  • Use the className above it to nest the CSS.

ref={ tabsRef }
onSelect={ handleSetSelectedTab }
onClose={ onClose }
Expand Down
17 changes: 15 additions & 2 deletions packages/block-editor/src/components/tabbed-sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import clsx from 'clsx';

/**
* WordPress dependencies
*/
Expand All @@ -16,11 +21,19 @@ import { unlock } from '../../lock-unlock';
const { Tabs } = unlock( componentsPrivateApis );

function TabbedSidebar(
{ defaultTabId, onClose, onSelect, selectedTab, tabs, closeButtonLabel },
{
defaultTabId,
onClose,
onSelect,
selectedTab,
tabs,
closeButtonLabel,
className,
},
ref
) {
return (
<div className="block-editor-tabbed-sidebar">
<div className={ clsx( 'block-editor-tabbed-sidebar', className ) }>
<Tabs
selectOnMove={ false }
defaultTabId={ defaultTabId }
Expand Down
4 changes: 0 additions & 4 deletions packages/block-editor/src/components/tabbed-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@
flex-direction: column;
flex-grow: 1;
overflow: hidden;

@include break-medium() {
width: 350px;
}
}

.block-editor-tabbed-sidebar__tablist-and-close-button {
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/list-view-sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export default function ListViewSidebar() {
ref={ sidebarRef }
>
<TabbedSidebar
className="block-editor__tabbed-sidebar"
tabs={ [
{
name: 'list-view',
Expand Down
5 changes: 5 additions & 0 deletions packages/editor/src/components/list-view-sidebar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,8 @@
}
}

.block-editor__tabbed-sidebar {
@include break-medium() {
width: 350px;
}
}
Comment on lines +52 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style also defines the inserter sidebar's width, so it feels like it's blurring the lines of the inserter and list view.

49 changes: 0 additions & 49 deletions packages/editor/src/components/sidebar/header.js
View file
Open in desktop

This file was deleted.

108 changes: 61 additions & 47 deletions packages/editor/src/components/sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,13 @@
import {
BlockInspector,
store as blockEditorStore,
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';
import { useSelect, useDispatch } from '@wordpress/data';
import {
Platform,
useCallback,
useContext,
useEffect,
useRef,
} from '@wordpress/element';
import { isRTL, __ } from '@wordpress/i18n';
import { Platform, useCallback, useEffect, useRef } from '@wordpress/element';
import { isRTL, __, _x } from '@wordpress/i18n';
import { drawerLeft, drawerRight } from '@wordpress/icons';
import { store as keyboardShortcutsStore } from '@wordpress/keyboard-shortcuts';
import { privateApis as componentsPrivateApis } from '@wordpress/components';
import { store as interfaceStore } from '@wordpress/interface';

/**
Expand All @@ -28,7 +22,6 @@ import PluginSidebar from '../plugin-sidebar';
import PostSummary from './post-summary';
import PostTaxonomiesPanel from '../post-taxonomies/panel';
import PostTransformPanel from '../post-transform-panel';
import SidebarHeader from './header';
scruffian marked this conversation as resolved.
Show resolved Hide resolved
import TemplateContentPanel from '../template-content-panel';
import TemplatePartContentPanel from '../template-part-content-panel';
import useAutoSwitchEditorSidebars from '../provider/use-auto-switch-editor-sidebars';
Expand All @@ -41,7 +34,7 @@ import {
TEMPLATE_POST_TYPE,
} from '../../store/constants';

const { Tabs } = unlock( componentsPrivateApis );
const { TabbedSidebar } = unlock( blockEditorPrivateApis );

const SIDEBAR_ACTIVE_BY_DEFAULT = Platform.select( {
web: true,
Expand All @@ -54,12 +47,9 @@ const SidebarContent = ( {
renderingMode,
onActionPerformed,
extraPanels,
onSelect,
} ) => {
const tabListRef = useRef( null );
// Because `PluginSidebar` renders a `ComplementaryArea`, we
// need to forward the `Tabs` context so it can be passed through the
// underlying slot/fill.
const tabsContextValue = useContext( Tabs.Context );

// This effect addresses a race condition caused by tabbing from the last
// block in the editor into the settings sidebar. Without this effect, the
Expand Down Expand Up @@ -88,14 +78,20 @@ const SidebarContent = ( {
}
}, [ tabName ] );

const { documentLabel } = useSelect( ( select ) => {
const { getPostTypeLabel } = select( editorStore );

return {
// translators: Default label for the Document sidebar tab, not selected.
documentLabel: getPostTypeLabel() || _x( 'Document', 'noun' ),
};
}, [] );

const { disableComplementaryArea } = useDispatch( interfaceStore );

return (
<PluginSidebar
identifier={ tabName }
header={
<Tabs.Context.Provider value={ tabsContextValue }>
<SidebarHeader ref={ tabListRef } />
</Tabs.Context.Provider>
}
closeLabel={ __( 'Close Settings' ) }
// This classname is added so we can apply a corrective negative
// margin to the panel.
Expand All @@ -107,22 +103,43 @@ const SidebarContent = ( {
toggleShortcut={ keyboardShortcut }
icon={ isRTL() ? drawerLeft : drawerRight }
isActiveByDefault={ SIDEBAR_ACTIVE_BY_DEFAULT }
header={ false }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If header is false, do we still need the headerClassName?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the closeLabel here either.

>
<Tabs.Context.Provider value={ tabsContextValue }>
<Tabs.TabPanel tabId={ sidebars.document } focusable={ false }>
<PostSummary onActionPerformed={ onActionPerformed } />
<PluginDocumentSettingPanel.Slot />
<TemplateContentPanel renderingMode={ renderingMode } />
<TemplatePartContentPanel />
<PostTransformPanel />
<PostTaxonomiesPanel />
<PatternOverridesPanel />
{ extraPanels }
</Tabs.TabPanel>
<Tabs.TabPanel tabId={ sidebars.block } focusable={ false }>
<BlockInspector />
</Tabs.TabPanel>
</Tabs.Context.Provider>
<TabbedSidebar
tabs={ [
{
name: sidebars.document,
title: documentLabel,
panel: (
<>
<PostSummary
onActionPerformed={ onActionPerformed }
/>
<PluginDocumentSettingPanel.Slot />
<TemplateContentPanel
renderingMode={ renderingMode }
/>
<TemplatePartContentPanel />
<PostTransformPanel />
<PostTaxonomiesPanel />
<PatternOverridesPanel />
{ extraPanels }
</>
),
},
{
name: sidebars.block,
title: __( 'Block' ),
panel: <BlockInspector />,
},
] }
onClose={ () => disableComplementaryArea( 'core' ) }
onSelect={ onSelect }
selectedTab={ tabName }
defaultTabId={ sidebars.document }
ref={ tabListRef }
closeButtonLabel={ __( 'Close Settings' ) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous label included the shortcut was Close Settings. ⇧⌘,

/>
</PluginSidebar>
);
};
Expand All @@ -142,6 +159,7 @@ const Sidebar = ( { extraPanels, onActionPerformed } ) => {
sidebars.document,
].includes( sidebar );
let _tabName = sidebar;

if ( ! _isEditorSidebarOpened ) {
_tabName = !! select(
blockEditorStore
Expand Down Expand Up @@ -176,20 +194,16 @@ const Sidebar = ( { extraPanels, onActionPerformed } ) => {
);

return (
<Tabs
<SidebarContent
tabName={ tabName }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tab-related data here should get moved within the SidebarContent rather than passed as props. I think it was done here previously because the Tabs needed be wrapping the SidebarContent. Now that we have a different structure, I think we could move it within.

keyboardShortcut={ keyboardShortcut }
showSummary={ showSummary }
renderingMode={ renderingMode }
onActionPerformed={ onActionPerformed }
extraPanels={ extraPanels }
selectedTabId={ tabName }
onSelect={ onTabSelect }
selectOnMove={ false }
>
<SidebarContent
tabName={ tabName }
keyboardShortcut={ keyboardShortcut }
showSummary={ showSummary }
renderingMode={ renderingMode }
onActionPerformed={ onActionPerformed }
extraPanels={ extraPanels }
/>
</Tabs>
/>
);
};

Expand Down
86 changes: 45 additions & 41 deletions packages/interface/src/components/complementary-area/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,47 +295,51 @@ function ComplementaryArea( {
scope={ scope }
id={ identifier.replace( '/', ':' ) }
>
<ComplementaryAreaHeader
className={ headerClassName }
closeLabel={ closeLabel }
onClose={ () => disableComplementaryArea( scope ) }
smallScreenTitle={ smallScreenTitle }
toggleButtonProps={ {
label: closeLabel,
size: 'small',
shortcut: toggleShortcut,
scope,
identifier,
} }
>
{ header || (
<>
<h2 className="interface-complementary-area-header__title">
{ title }
</h2>
{ isPinnable && (
<Button
className="interface-complementary-area__pin-unpin-item"
icon={ isPinned ? starFilled : starEmpty }
label={
isPinned
? __( 'Unpin from toolbar' )
: __( 'Pin to toolbar' )
}
onClick={ () =>
( isPinned ? unpinItem : pinItem )(
scope,
identifier
)
}
isPressed={ isPinned }
aria-expanded={ isPinned }
size="compact"
/>
) }
</>
) }
</ComplementaryAreaHeader>
{ header !== false && (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only change here - if header is explicitly set to false then we don't output it. This might break other things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a better way to do this either.

<ComplementaryAreaHeader
className={ headerClassName }
closeLabel={ closeLabel }
onClose={ () => disableComplementaryArea( scope ) }
smallScreenTitle={ smallScreenTitle }
toggleButtonProps={ {
label: closeLabel,
size: 'small',
shortcut: toggleShortcut,
scope,
identifier,
} }
>
{ header || (
<>
<h2 className="interface-complementary-area-header__title">
{ title }
</h2>
{ isPinnable && (
<Button
className="interface-complementary-area__pin-unpin-item"
icon={
isPinned ? starFilled : starEmpty
}
label={
isPinned
? __( 'Unpin from toolbar' )
: __( 'Pin to toolbar' )
}
onClick={ () =>
( isPinned ? unpinItem : pinItem )(
scope,
identifier
)
}
isPressed={ isPinned }
aria-expanded={ isPinned }
size="compact"
/>
) }
</>
) }
</ComplementaryAreaHeader>
) }
<Panel className={ panelClassName }>{ children }</Panel>
</ComplementaryAreaFill>
</>
Expand Down
Loading