Sidebar: Add a shared component for the inserter and list view #62343
Conversation
|
Size Change: -8.85 kB (-0.5%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a7a65e7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9761003448
|
scruffian
force-pushed
the
refactor/tabbed-sidebar
branch
from
ff180fd to
236d52a
Compare
|
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. |
|
I'd be totally in favor of a shared component to reduce duplication and have more consistency. Worth noting the Inserter should not use an ARIA tabs interface when it shows only one kind of object. That's the case in the Widgets page and in the Customizer-widgets panel which only show Blocks. Could this component render an alternative ui with no ARIA tabs semantics when there's only one kind of objects? The implementation that as in place before the recent editor reunification effort did use an alternative UI for this case but that went lost. |
|
Inner styles should not be overriden for Separately, there are some valid arguments against these full-width styles, like #61974 (comment) While I think a component like this that unifies styles is a good addition (without having looked too deep into the details), I think the conversation in #61974 should be resolved first before committing to one or the other. If full-width is the decision, then there should be work to implement those styles in |
|
We are already overriding these styles in the individual components, so this doesn't break that convention any more than it's already been broken, it just centralizes that code. |
|
@scruffian I think that's reasonable. Please note that the first example in the description has an intentional design detail where the tabs indicator overlaps the border below it, as can be seen in the "before" image. If we unify all of these (and the ones you mention for the future), we should also make a unified decision regarding that detail. cc @WordPress/gutenberg-design |
|
Good catch, @DaniGuardiola, that is definitely intentional, and is about ensuring legibility of the active state, so that's an important detail to retain. |
| <Button | ||
| className="block-editor-tabbed-sidebar__close-button" | ||
| icon={ closeSmall } | ||
| label={ __( 'Close block inserter' ) } |
There was a problem hiding this comment.
This will need to be dynamic, as the list view close button tooltip also says "Close block inserter"
| @@ -162,6 +162,7 @@ export { default as __experimentalPublishDateTimePicker } from './publish-date-t | |||
| export { default as __experimentalInspectorPopoverHeader } from './inspector-popover-header'; | |||
| export { default as BlockPopover } from './block-popover'; | |||
| export { useBlockEditingMode } from './block-editing-mode'; | |||
| export { default as TabbedSidebar } from './tabbed-sidebar'; | |||
There was a problem hiding this comment.
This is a helpful refactor to work towards unified sidebars, so I don't want to block this progress. That said, I think it will be important to not combine the content of the sidebar with the interaction of the sidebar wrapper. I see the eventual goal as having a shared sidebar component (or another name that does not reference the visual position of the component) with a close button. The content within it can be whatever it needs to be.
This will be possible if we move to a header title for each sidebar, as proposed in #61553
packages/block-editor/README.md
Outdated
| @@ -804,6 +804,10 @@ _Related_ | |||
|
|
|||
| - <https://github.com/WordPress/gutenberg/blob/HEAD/packages/data/README.md#registerStore> | |||
|
|
|||
| ### TabbedSidebar | |||
There was a problem hiding this comment.
Should this be a public component?
| <TabbedSidebar | ||
| tabs={ [ | ||
| { | ||
| name: 'slug-1', | ||
| title: _x( 'Title 1', 'context' ), | ||
| panel: <PanelContents>, | ||
| panelRef: useRef('an-optional-ref'), | ||
| }, | ||
| { | ||
| name: 'slug-2', | ||
| title: _x( 'Title 2', 'context' ), | ||
| panel: <PanelContents />, | ||
| }, | ||
| ] } | ||
| onClose={ onClickCloseButton } | ||
| onSelect={ onSelectTab } | ||
| defaultTabId="slug-1" | ||
| ref={ tabsRef } | ||
| /> |
There was a problem hiding this comment.
To elaborate on the above, I think this should eventually look more like:
<StickyPanel title="Block Inserter" onClose={ onClickCloseButton }>
<Tabs>
...
</Tabs>
</StickyPanel>
Name tbd 😬 😅
There was a problem hiding this comment.
Agree on this. We should try to keep things composable to avoid "YAP" syndrome.
|
Just wanna say two things:
|
|
@DaniGuardiola - to improve the accessibility, we'd need design agreement for adding a header with title and close button to all the sidebars. I would love if this could get momentum. It has agreement and support from folks on the WordPress Accessibility team. |
|
@jeryj love that! This PR is also a great chance to add that as a follow-up, and get it going in all sidebars. |
|
Based on the discussion on the linked PR, it seems a consensus is going to be hard to reach for that design. Can we instead focus on the unification part in this PR while we continue iterating on the design separately. |
|
I agree with @youknowriad, let's leave the visual design discussion separate from this PR. Let's still make sure this component is accessible to all users, please :) |
While I think we should ensure the component is accessible, I agree with @youknowriad and believe any accessibility improvements could be made separately as they aren't necessarily part of the work to unify the existing components in question. |
|
Agreed :) I think any changes to the styles should be a follow-up. After this is merged, can this tabbed sidebar component be applied to the Settings panel as well? |
Thanks. I have moved the indicator so it overlaps the border and updated the screenshots. |
|
I have updated this based on the feedback above. I have made it a locked component to give us time to iterate on it.
Definitely |
There was a problem hiding this comment.
I think this is better than what we have since it consolidates the code more and can also be applied to the Settings sidebar. However, I do have concerns over how the Tabs are tied to a close button that controls the sidebar outside of it. It feels like two unrelated things are being forced together. But, pragmatically, this is better than what we have and the component is private so we should be able to refactor it when the time comes.
|
I'm going to merge this so that we can continue to align the various panels to use this component and ensure that improvments are made to all at once. I am very happy for others to jump in and make improvments - for example making the component more composable etc. |
…ress#62343) * remove the old panels * move styles * rename classes * trying to fix the overflow * remove will-change: transform * update docs * add the correct markup for the list view * remove padding * remove unsued code * remove unused code * remove unused comment * move styles across * readd height * remove more code * fix the position of patterns * use different labels for each close button * Make TabbedSidebar private Co-authored-by: scruffian <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: DaniGuardiola <[email protected]> Co-authored-by: afercia <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: tyxla <[email protected]>
What?
Creates a new
TabbedPanelcomponent that is used by both the inserter and the list view.Why?
This will ensure visual and behaviour consistency between these two panels. In time we should also update the block inspector and global styles to use the component too.
How?
TabbedPanelcomponentTesting Instructions
Testing Instructions for Keyboard
Also check that the keyboard interactions are the same as in trunk.
Note: There is ongoing discussion about the best way to handle keyboard interactions with these panels. This PR aims to consolidate these panels to use the same code and approach. When we update the approach to keyboard interactions then we'll only need to do it in one place for all panels.
Screenshots or screencast
|
| 
|
The visual differences are: