Inspector: Use the TabbedSidebar component #63184
Open
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
scruffian
added
the
[Feature] Inspector Controls
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
scruffian
commented
Jul 5, 2024
| @@ -316,6 +316,7 @@ function InserterMenu( | |||
| > | |||
| <div className="block-editor-inserter__main-area"> | |||
| <TabbedSidebar | |||
| className="block-editor__tabbed-sidebar" | |||
There was a problem hiding this comment.
We need this so that we can set a width for only the inserter a the list view.
There was a problem hiding this comment.
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.
scruffian
commented
Jul 5, 2024
scruffian
commented
Jul 5, 2024
scruffian
commented
Jul 5, 2024
scruffian
commented
Jul 5, 2024
youknowriad
reviewed
Jul 5, 2024
There was a problem hiding this comment.
It feels like more code to use a shared component rather than avoid it? Isn't that an issue?
isn't the hideHeader the same thing as passing an empty header?
Just wondering if we're actually building the right abstraction basically or if we're bending something to use an abstraction.
IMO The reason for the abstraction is less about minimizing code and more about having consistent UX. At the moment each of these panels looks and behaves differently, and even when we have made efforts to get them in sync they have moved apart again. The hope is that by using the same component we'll have the same UX for all panels. |
|
Incidentally now I have removed the
|
No, if you don't pass a header then the |
|
Size Change: -59 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
scruffian
commented
Jul 5, 2024
| </> | ||
| ) } | ||
| </ComplementaryAreaHeader> | ||
| { header !== false && ( |
There was a problem hiding this comment.
This is the only change here - if header is explicitly set to false then we don't output it. This might break other things.
There was a problem hiding this comment.
I don't see a better way to do this either.
jeryj
reviewed
Jul 9, 2024
| selectedTab={ tabName } | ||
| defaultTabId={ sidebars.document } | ||
| ref={ tabListRef } | ||
| closeButtonLabel={ __( 'Close Settings' ) } |
There was a problem hiding this comment.
The previous label included the shortcut was Close Settings. ⇧⌘,
| @@ -107,22 +103,43 @@ const SidebarContent = ( { | |||
| toggleShortcut={ keyboardShortcut } | |||
| icon={ isRTL() ? drawerLeft : drawerRight } | |||
| isActiveByDefault={ SIDEBAR_ACTIVE_BY_DEFAULT } | |||
| header={ false } | |||
There was a problem hiding this comment.
If header is false, do we still need the headerClassName?
There was a problem hiding this comment.
I don't think we need the closeLabel here either.
| @@ -316,6 +316,7 @@ function InserterMenu( | |||
| > | |||
| <div className="block-editor-inserter__main-area"> | |||
| <TabbedSidebar | |||
| className="block-editor__tabbed-sidebar" | |||
There was a problem hiding this comment.
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.
Comment on lines
+52
to
+56
| .block-editor__tabbed-sidebar { | ||
| @include break-medium() { | ||
| width: 350px; | ||
| } | ||
| } |
There was a problem hiding this comment.
This style also defines the inserter sidebar's width, so it feels like it's blurring the lines of the inserter and list view.
|
Regarding the X Close button order, pleae see my comment at #59013 (comment) |
jeryj
reviewed
Jul 10, 2024
| @@ -176,20 +194,16 @@ const Sidebar = ( { extraPanels, onActionPerformed } ) => { | |||
| ); | |||
|
|
|||
| return ( | |||
| <Tabs | |||
| <SidebarContent | |||
| tabName={ tabName } | |||
There was a problem hiding this comment.
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.
|
The current mobile view has tabs that are quite wide. On 
|
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?
Implements the new TabbedSidebar component in the Inspector Controls
Why?
For consistency it's good to use the same component for all our tabbed sidebars.
TabbedSidebarsis used for the List View and the Inserter, so this adds it to the Inspector as well.How?
Refactoring to use
TabbedSidebar.Testing Instructions
Screenshots or screencast