Admin UI: Add Storybook stories for Breadcrumbs and Page components#76467
Admin UI: Add Storybook stories for Breadcrumbs and Page components#76467ciampo merged 7 commits intoWordPress:trunkfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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. |
simison
added
Storybook
|
Outside the specific contents of the stories, thanks for adding these! What's below isn't a blocker, but worth considering as this introduces a new top level folder. And we don't have to get this right immediately, because we can easily update the URLs at a later time. But it is worth thinking about as Storybook grows. I've been doing a little storybook taxonomical cleanup, with the goal of making componentry more easily discoverable. At the moment, it is somewhat overwhelmingly component-focused, to the point that things like Destructive Actions is hard to find. One mitigation is to start all root folders collapsed, another is to consolidate and better group top level folders in terms of categorization (just one example). In case of this PR, I'm not sure "Admin UI" should be a top level folder. Is there a subfolder we can group it in? But if it has to be, it's worth consolidating the others. One proposal could be:
This is the more conservative approach, that's still relatively dev-forward with package representation. If we were okay with more nested subfolders, we could arguably be more generic. |
|
I totally agree the Storybook sidebar needs re-organising, but I think that it needs a concerted effort and is best handled elsewhere. If we didn't want to place Admin UI at the root just yet then I'd probably vote for adding it as a folder inside the "Design system" section. @CGastrell one small change request; in the |
Agree it shouldn't block. However if we already now know we'll remove the "Design System" folder, then we shouldn't place it inside. I think it's worth agreeing: is it a top level folder or not, and if it's not a top level folder, then what folder should it be in? Easiest way to unblock and merge this is to just be fine that it's a top level folder for now, and to your own point, do the concerted effort for organising later. |
|
I don't have a strong feeling without evaluating the full structure, which shouldn't happen in this PR. So I agree a root-level item is okay for now. |
|
Thanks for the comments and considerations. Yes, I didn't check any organization regarding the sidebar menu, might as well just make it part of another main entry. I wrapped the dummy content with While it's always good to have our components available on the Storybook, the main rationale for this is that we'll be doing more development on these components and testing will usually involve having to mess with current implementations. With this in place, development and iteration should improve drastically. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Since these are the |
It looks like the Maybe we could mock Separately, I've opened #76493 to tighten prop types around the expected shape of |
Move admin-ui stylesheet loading from direct imports in story files to the storybook/package-styles config system for proper isolation, dependency management, and RTL support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ThemeProvider is not needed since styles are loaded via the package-styles config system. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
So Link from @wordpress/route is literally a re-export of Link from @tanstack/react-router. It's the actual tanstack Link component — there's no wrapper or abstraction layer. This means mocking it isn't straightforward in Storybook without a module alias/mock setup. The Link requires a router context to function. We could mock @wordpress/route at the Storybook level (via Vite aliases in storybook/main.ts), but that's a broader change that would affect all stories importing from @wordpress/route. The RouterContextProvider approach is the most direct and accurate — it tests the actual component as-is. A mock would hide real rendering behavior. I'd say the current approach is the pragmatic choice, and the alternative would require configuring a Storybook-wide module mock, which is more complex for arguably less fidelity. |
Other than these changes, I don't see a simple way to mock |
There definitely are ways, but I'm thinking that we should discuss them separately — other approaches (for example, mocking @wordpress/route via a scoped Vite plugin) would have different trafe-offs, and I'm not 100% about what's the best approach. That said, a couple of improvements we can still make:
These changes work on my machine, although I'd like to hear a second opinion (maybe from @mirka )diff --git c/packages/admin-ui/package.json i/packages/admin-ui/package.json
index 1fedf245fc..9a183ddb40 100644
--- c/packages/admin-ui/package.json
+++ i/packages/admin-ui/package.json
@@ -49,6 +49,7 @@
"@wordpress/components": "file:../components",
"@wordpress/element": "file:../element",
"@wordpress/i18n": "file:../i18n",
+ "@wordpress/private-apis": "file:../private-apis",
"@wordpress/route": "file:../route",
"@wordpress/ui": "file:../ui",
"clsx": "^2.1.1"
diff --git c/packages/admin-ui/src/breadcrumbs/stories/index.story.tsx i/packages/admin-ui/src/breadcrumbs/stories/index.story.tsx
index f67a82b195..886fa71dcb 100644
--- c/packages/admin-ui/src/breadcrumbs/stories/index.story.tsx
+++ i/packages/admin-ui/src/breadcrumbs/stories/index.story.tsx
@@ -2,37 +2,17 @@
* External dependencies
*/
import type { Meta, StoryObj } from '@storybook/react-vite';
-import {
- createMemoryHistory,
- createRootRoute,
- createRouter,
- RouterContextProvider,
-} from '@tanstack/react-router';
/**
* Internal dependencies
*/
import Breadcrumbs from '..';
-
-const rootRoute = createRootRoute( {
- notFoundComponent: () => null,
-} );
-const router = createRouter( {
- routeTree: rootRoute,
- history: createMemoryHistory( { initialEntries: [ '/' ] } ),
- defaultNotFoundComponent: () => null,
-} );
+import { withRouter } from '../../stories/with-router';
const meta: Meta< typeof Breadcrumbs > = {
component: Breadcrumbs,
title: 'Admin UI/Breadcrumbs',
- decorators: [
- ( Story ) => (
- <RouterContextProvider router={ router }>
- <Story />
- </RouterContextProvider>
- ),
- ],
+ decorators: [ withRouter ],
};
export default meta;
diff --git c/packages/admin-ui/src/lock-unlock.ts i/packages/admin-ui/src/lock-unlock.ts
new file mode 100644
index 0000000000..88024b42bc
--- /dev/null
+++ i/packages/admin-ui/src/lock-unlock.ts
@@ -0,0 +1,10 @@
+/**
+ * WordPress dependencies
+ */
+import { __dangerousOptInToUnstableAPIsOnlyForCoreModules } from '@wordpress/private-apis';
+
+export const { lock, unlock } =
+ __dangerousOptInToUnstableAPIsOnlyForCoreModules(
+ 'I acknowledge private features are not for use in themes or plugins and doing so will break in the next version of WordPress.',
+ '@wordpress/admin-ui'
+ );
diff --git c/packages/admin-ui/src/page/stories/index.story.tsx i/packages/admin-ui/src/page/stories/index.story.tsx
index c47468452a..7b1e6a4dd1 100644
--- c/packages/admin-ui/src/page/stories/index.story.tsx
+++ i/packages/admin-ui/src/page/stories/index.story.tsx
@@ -2,12 +2,6 @@
* External dependencies
*/
import type { Meta, StoryObj } from '@storybook/react-vite';
-import {
- createMemoryHistory,
- createRootRoute,
- createRouter,
- RouterContextProvider,
-} from '@tanstack/react-router';
import { __experimentalText as Text } from '@wordpress/components';
/**
@@ -15,15 +9,7 @@ import { __experimentalText as Text } from '@wordpress/components';
*/
import Page from '..';
import Breadcrumbs from '../../breadcrumbs';
-
-const rootRoute = createRootRoute( {
- notFoundComponent: () => null,
-} );
-const router = createRouter( {
- routeTree: rootRoute,
- history: createMemoryHistory( { initialEntries: [ '/' ] } ),
- defaultNotFoundComponent: () => null,
-} );
+import { withRouter } from '../../stories/with-router';
const meta: Meta< typeof Page > = {
component: Page,
@@ -33,12 +19,11 @@ const meta: Meta< typeof Page > = {
},
decorators: [
( Story ) => (
- <RouterContextProvider router={ router }>
- <div style={ { height: '400px' } }>
- <Story />
- </div>
- </RouterContextProvider>
+ <div style={ { height: '400px' } }>
+ <Story />
+ </div>
),
+ withRouter,
],
};
diff --git c/packages/admin-ui/src/stories/with-router.tsx i/packages/admin-ui/src/stories/with-router.tsx
new file mode 100644
index 0000000000..2aa5563ed1
--- /dev/null
+++ i/packages/admin-ui/src/stories/with-router.tsx
@@ -0,0 +1,29 @@
+/**
+ * WordPress dependencies
+ */
+import { privateApis as routePrivateApis } from '@wordpress/route';
+
+/**
+ * Internal dependencies
+ */
+import { unlock } from '../lock-unlock';
+
+const { createMemoryHistory, createRootRoute, createRouter, RouterProvider } =
+ unlock( routePrivateApis );
+
+/**
+ * Storybook decorator that provides a router context.
+ *
+ * Wraps stories in a minimal tanstack router (via @wordpress/route private
+ * APIs) so that components consuming `Link` from `@wordpress/route` can
+ * render without errors.
+ */
+export function withRouter( Story: React.ComponentType ) {
+ const rootRoute = createRootRoute( { component: Story } );
+ const router = createRouter( {
+ routeTree: rootRoute,
+ history: createMemoryHistory( { initialEntries: [ '/' ] } ),
+ defaultNotFoundComponent: () => null,
+ } );
+ return <RouterProvider router={ router } />;
+}
diff --git c/packages/admin-ui/tsconfig.json i/packages/admin-ui/tsconfig.json
index 4bc8c7cf87..5f3610ded0 100644
--- c/packages/admin-ui/tsconfig.json
+++ i/packages/admin-ui/tsconfig.json
@@ -5,6 +5,7 @@
{ "path": "../components" },
{ "path": "../element" },
{ "path": "../i18n" },
+ { "path": "../private-apis" },
{ "path": "../route" },
{ "path": "../ui" }
]
diff --git c/packages/private-apis/src/implementation.ts i/packages/private-apis/src/implementation.ts
index 6f50a88055..21c9e4d1c0 100644
--- c/packages/private-apis/src/implementation.ts
+++ i/packages/private-apis/src/implementation.ts
@@ -10,6 +10,7 @@
* The list of core modules allowed to opt-in to the private APIs.
*/
const CORE_MODULES_USING_PRIVATE_APIS = [
+ '@wordpress/admin-ui',
'@wordpress/block-directory',
'@wordpress/block-editor',
'@wordpress/block-library',
diff --git c/packages/route/src/private-apis.ts i/packages/route/src/private-apis.ts
index 9b2735cbc9..ff249316c3 100644
--- c/packages/route/src/private-apis.ts
+++ i/packages/route/src/private-apis.ts
@@ -6,6 +6,7 @@ import {
createBrowserHistory,
createLazyRoute,
createLink,
+ createMemoryHistory,
createRootRoute,
createRoute,
createRouter,
@@ -37,6 +38,7 @@ export const privateApis = {};
lock( privateApis, {
// Router creation and setup
createBrowserHistory,
+ createMemoryHistory,
createLazyRoute,
createRouter,
createRootRoute,
|
Sounds good, I can add this if you think is best, let me wait for @mirka as I'm not so experienced on the repo or what justifies a helper/decorator. |
…private APIs Replace direct @tanstack/react-router imports with a shared withRouter decorator that uses @wordpress/route private APIs, keeping consistent with Gutenberg conventions. See: WordPress#76467 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| title: 'Page title', | ||
| showSidebarToggle: false, | ||
| children: ( | ||
| <Text style={ { padding: '24px 24px' } }>Page content here</Text> |
There was a problem hiding this comment.
| <Text style={ { padding: '24px 24px' } }>Page content here</Text> | |
| <Text>Page content here</Text> |
Not a blocker, but I don't think that consumers of Page should have to pass a custom padding every time they use the component. Ideally, the main spacing should be provided by Page and the consumers should only worry about content + intra-content spacing
(here and everywhere else in the PR)
There was a problem hiding this comment.
I agree with you on consumers not having to pass custom padding, but this is how it looks without the injected padding:
Do you know if we have some container for this purpose already? Should the Page component handle this so its children are already contained with paddings?
There was a problem hiding this comment.
FWIW, we can leave it with or without the injected paddings while we figure out whose responsibility is the children's paddings.
There was a problem hiding this comment.
I took a look at the Page component, and it has a dedicated hasPadding prop, which serves exactly that purpose.
Now, we normally code stories so that they use as many default prop values as possible (and hasPadding is false by default), so I suggest that at least:
- if we want to keep the
paddingon the default story, we change it to a shorthand (ie just24px) and add a code comment explaining that, by default,Pagedoesn't add padding to its content - we could have a dedicated
with paddingstory
Also curious to hear @jameskoster and @mirka 's thoughts on this one
There was a problem hiding this comment.
I think we ought to consider reusing the technique from Card. IE Content and FullBleed sub-components that handle padding (Content would have padding applied). This seems a more flexible solution compared to unilaterally saying the contents are padded or not.
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Let's add the min height back for the Page stories:
diff --git i/packages/admin-ui/src/page/stories/index.story.tsx w/packages/admin-ui/src/page/stories/index.story.tsx
index 4be16b01f80..a8544ce8605 100644
--- i/packages/admin-ui/src/page/stories/index.story.tsx
+++ w/packages/admin-ui/src/page/stories/index.story.tsx
@@ -17,7 +17,14 @@ const meta: Meta< typeof Page > = {
parameters: {
layout: 'fullscreen',
},
- decorators: [ withRouter ],
+ decorators: [
+ ( Story ) => (
+ <div style={ { minHeight: '400px' } }>
+ <Story />
+ </div>
+ ),
+ withRouter,
+ ],
};
export default meta;
Also, you'll need to run npm i on your maching and push the changes to the lockfile (it should be a one line change)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
I think we're good to merge, and iterate in follow-ups as necessary 🚀
In particular, I'd like to improve devX and separation of concerns on the the padding
…#76467) * Admin UI: Add Storybook stories for Breadcrumbs and Page components Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Admin UI: Use Text component for Page story content Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Admin UI: Use package-styles config for Storybook style loading Move admin-ui stylesheet loading from direct imports in story files to the storybook/package-styles config system for proper isolation, dependency management, and RTL support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Admin UI: Remove ThemeProvider from Storybook stories ThemeProvider is not needed since styles are loaded via the package-styles config system. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Admin UI: Extract shared withRouter decorator using @wordpress/route private APIs Replace direct @tanstack/react-router imports with a shared withRouter decorator that uses @wordpress/route private APIs, keeping consistent with Gutenberg conventions. See: WordPress/gutenberg#76467 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Admin UI: Add min-height to Page stories decorator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Update package-lock.json for @wordpress/private-apis dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: CGastrell <cgastrell@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: simison <simison@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Source: WordPress/gutenberg@ec5ccf0
6 participants
What?
Adds Storybook stories for the
BreadcrumbsandPagecomponents in the@wordpress/admin-uipackage.See #76448
Why?
The admin-ui package currently has no Storybook coverage for its components. Adding stories enables visual development, design review, and documentation for these components as work continues on the Admin UI visual tweaks tracked in #76448.
How?
packages/admin-uiinstorybook/main.tsSingleItem,TwoLevels,ThreeLevels— demonstrating breadcrumb links and heading levelsDefault,WithSubtitle,WithBreadcrumbs,WithBreadcrumbsAndSubtitle— demonstrating the Page layout with various header configurationsThemeProvider(from@wordpress/theme) andRouterContextProvider(from@tanstack/react-router) to match the runtime environmentTesting Instructions
npm run storybook:devSingleItem: single breadcrumb, no separatorTwoLevels: first item is a link, second is a heading, separated by/ThreeLevels: first two items are links, last is a heading, separated by/Default: page with titleWithSubtitle: page with title and subtitleWithBreadcrumbs: page with breadcrumb navigation (first breadcrumb is a link)WithBreadcrumbsAndSubtitle: page with breadcrumbs and subtitle togetherTesting Instructions for Keyboard
navelement has an appropriatearia-label("Breadcrumbs")