Admin UI: Add visual prop to Page header component

[CGastrell]

See design convo #76709

What?

Adds a visual prop to the Page component's header, allowing consumers to display a logo or symbol alongside the title or breadcrumbs.

See #76448

Note: This PR depends on #76467 (Storybook stories for admin-ui). The story files in this diff include both the base stories from that PR and the new logo stories. The implementation changes are in header.tsx, index.tsx, and style.scss.

Why?

As described in #76448, consumers currently pass logos inside the title prop, which places them inside the h2 tag. This is fragile and creates accessibility issues — when the title contains non-text content, a separate ariaLabel must be passed to maintain good semantics (see #75898).

An officially supported visual prop renders the logo outside the h2 tag, ensuring:

• Consistent logo placement across consumers
• Proper semantic structure (visual is not part of the heading tag)
• No need for a separate ariaLabel if the only difference is the visual between title and ariaLabel.

How?

• Added visual?: React.ReactNode prop to Page (index.tsx) and Header (header.tsx)
• Logo renders before the title/breadcrumbs in the header row, wrapped in a div.admin-ui-page__header-logo
• Added CSS for the logo container: flex-centered, non-shrinking, with object-fit: contain for img/svg children
• Added Storybook stories demonstrating the logo with title, with breadcrumbs, and with a Jetpack logo

Testing Instructions

1. Run npm run storybook:dev
2. Navigate to Admin UI > Page in the sidebar
3. Check the following stories:
   • With Logo: globe icon + "Page title" heading
   • With Jetpack Logo: Jetpack icon + "Jetpack" heading
   • With Logo And Breadcrumbs: globe icon + breadcrumb navigation
   • With Jetpack Logo And Breadcrumbs: Jetpack icon + breadcrumb navigation
4. Verify the logo appears to the left of the title/breadcrumbs
5. Inspect the DOM — the logo should be outside the h2 element

Testing Instructions for Keyboard

1. Tab through the stories — the logo container should not be focusable (it's decorative
2. The heading and breadcrumb links should remain keyboard accessible
   EOF
   )

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: CGastrell <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: simison <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: aduth <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[simison]

Thinking if icon as prop name instead of logo would be more aligned with other components (e.g. button)

[simison]

FYI I updated PR title+description to use icon instead of logo as done also in commit 09361c5

[ciampo]

Have you considered rendering an Icon component directly in the Header? What if consumers have different icon positioning needs? How would this interact in pages with different combinations of title / breadcrumbs (including none of them?

What I'm worried about is that we're introducing an abstraction that may not be flexible enough for future needs.

I'm also lacking the overall vision for Page and admin UI — rather than adding a little piece at a time and risk building the wrong abstractions, I think it would be better to take a step back, understand all the requirements, and figure out the best abstractions / implementations to satisfy them (cc @WordPress/gutenberg-components )

[mirka]

Yes, seeing the aria-label issue (#75899) too, there are already warning signs in the API that I would recommend considering a less monolithic approach, namely with subcomponents like Page.Header.

Warning signs: There are three props on root-level Page that are for the same conceptual element (heading), which is itself a sign that this architecture may not be tenable, and also the prop naming (aria-label — for what?, title, headingLevel) is already kind of haphazard and doesn't make those relationships clear.

[simison]

@mirka context for the recent ariaLabel and headingLevel additions:

• Connectors: Improve accessibility #76456
• Admin UI: Fix type mismatch between Page title and NavigableRegion ariaLabel #75899

Both are related to accessibility, but the flexibility of h1-h6 heading option seems kinda lot to me. I don't know enough about use-cases if heading ever needs to be anything else than h1; maybe?

[simison]

Noting that while it's good we're lookinga at different prop-combos and how they should work together in different variations, I'm not sure to what extend we should codify constraints and things always necessarily end up looking good. Consumers should have some responsibility to make sure e.g. this case continues to look good even if actual header is super narrow on full desktop width:

[CGastrell]

What if consumers have different icon positioning needs?

As far as I can tell, the structure of admin-ui's Page is to provide a consistent look throughout pages that will live on the site admin. This is where we're coming from, the multiplicity of pages and styles built on wp-admin.

Now, we want to be flexible, but we also want to set some sort of "standard". In that regard I don't see many abstractions, hence why Header is not (yet) exposed, it only exists through Page props. And it aims to always look the same.

Title, Breadcrumbs and Badges are all Page props that then get passed down (internally) to Header. If we want to add full flexibility, then this shouldn't happen and we'd need to expose/export Header, allowing consumers to:

<Page>
  <Header />
  <SomeOther />
</Page>

But that would still suffer the same consequences: consumer would need to compose Header in the same way it would need to compose Page.

Right now Header is the one sorting out how/when things are rendered and, in some cases, it even delegates or expects props to provide the right components (Breadcrumbs needs to be passed down, it's not constructed from an array).

So, with this same approach, the proposed icon prop is meant to receive a component and be placed consistently so that the page looks is retained.

Is this a constraint we want? I assumed yes. Otherwise the consumer can use its own component for the page they need. Page, from my PoV, exists to provide a unified way to build pages to integrate seamlessly into the admin. Whatever magic we put in there is only meant to keep design aligned, that means restrictions, yes, but also the benefit of having things work out of the box.

Now, back to the icon prop and other warning signs flagging this as a haphazard situation, that depends on the level of control Page should hold over its structural design. As @mirka points out, this turns to a monolithic approach, yet I can't be sure this is not what we want in this particular case. Composing Header through Page props seems as functional as exposing Header and allow this to be ruling its own props.

Shall a consumer want to fully customize its page they can, if Page doesn't receive any of the three props (title, breadcrumbs or badges, and now icon) the Header is not rendered and the consumer can enjoy a fully blank canvas to work on. This is the "off-road" option we can offer, but once a single of those props is provided, we want it to be consistent and Page uses Header to render it, keeping the flexbox behavior and alignment in this unified way.

That said, we might also be more worried about actions, that don't even get considered to render the Header, meaning <Page actions={ someActions } /> would never render those actions anywhere.

@mirka @ciampo I see the concern, and I agree this might be a completely different discussion on structural design of the component. I'm just saying that this decision will affect the current way we expose/render the Header and, once we come to that decision, it should be the same work to migrate any of those props as it would be to migrate icon.

[mirka]

Both are related to accessibility

Not questioning the necessity of the prop, but the naming (or API structure) here. A plain root-level aria-label prop on a Page component doesn't make clear what exactly the aria-label is applying to. I looked at the code and it's just for the wrapping region, which also was unexpected from the component name "Page" — it's actually a Region component? There will never be sibling regions on the "Page"?

Not just the unclarity, but it's problematic when you eventually need to accept custom aria-labels on other internal elements that are also covered by root-level props. How exhaustive are the props we have now (genuine question because I don't know)? There are root-level props applying to the region, to the header area, and to the content area, often without differentiators in the name. This will lead to prop bloat and strained APIs.

There's a balance to be found between ergonomics, consistency, and modularity, but we've found it easier to find that balance by starting from the fully modular, then at the end tweaking ergonomics and consistency in the monolithic component. Maybe that is what is happening here, because the code does show internal modularity, but the way props are all flatly exposed on the monolithic component gives me pause, if we're not close to exhausting our prop set yet.

And there are type-friendly, ergonomic ways to mitigate root-level prop bloat, for example <Page header={ <Page.Header /* pass all the header props here */ /> }>.

[CGastrell]

Both are related to accessibility

Not questioning the necessity of the prop, but the naming (or API structure) here. A plain root-level aria-label prop on a Page component doesn't make clear what exactly the aria-label is applying to. I looked at the code and it's just for the wrapping region, which also was unexpected from the component name "Page" — it's actually a Region component? There will never be sibling regions on the "Page"?

Not just the unclarity, but it's problematic when you eventually need to accept custom aria-labels on other internal elements that are also covered by root-level props. How exhaustive are the props we have now (genuine question because I don't know)? There are root-level props applying to the region, to the header area, and to the content area, often without differentiators in the name. This will lead to prop bloat and strained APIs.

There's a balance to be found between ergonomics, consistency, and modularity, but we've found it easier to find that balance by starting from the fully modular, then at the end tweaking ergonomics and consistency in the monolithic component. Maybe that is what is happening here, because the code does show internal modularity, but the way props are all flatly exposed on the monolithic component gives me pause, if we're not close to exhausting our prop set yet.

And there are type-friendly, ergonomic ways to mitigate root-level prop bloat, for example <Page header={ <Page.Header /* pass all the header props here */ /> }>.

Completely agree from a component design PoV. Though, this would be a breaking change, not for the sake of blocking it, just to make sure we take the necessary precautions before such change. On top of it, I like the <Page.Header /* props */ /> syntax, makes it very self explanatory on usage and scope 👍

Out of the scope of this PR but, anyone up to prepare such change?

[CGastrell]

BTW, fixing the actions bug: #76695

[jameskoster]

Outside of the excellent discussion so far I'd like to point out that the dimensions of the icon should be restricted somehow, and ideally an aspect ratio of 1/1 forced. If a user supplied a 1280x1024px jpg we wouldn't want it to render at that size. Even if a consumer supplied an Icon instance the size should ideally be pre-determined if possible.

#76709 suggests 20x20, but that won't play nicely with Icon which defaults to 24x24, so I'd suggest using 24px for consistency.

[ciampo]

LGTM 🚀

[Tropicalista]

To add custom image this need to be solved.

[manzoorwanijk]

@wordpress/icons is not listed as a dev dependency admin-ui.

It was caught in #75814 when the storybook build failed.

[aduth]

Shouldn't import/no-extraneous-dependencies be helping catch these issues? 🤔

[manzoorwanijk]

Is that enabled for TS files?

[aduth]

It looks like it's disabled for "development files", including Storybook files. I'm not sure why. If I had to speculate before diving further, my guess is that we historically defined devDependencies in the root package.json, and that ESLint rule can be tricky to configure for complex rules like that. If we agree that a package should define all of its own dependencies, we should probably be able to turn it back on.

gutenberg/eslint.config.mjs

Lines 321 to 336 in 68fd59c

	// Override: React Native and development files — disable certain import/data rules.
	{
	files: [
	'**/*.@(android|ios|native).js',
	'packages/react-native-*/**/*.js',
	...developmentFiles,
	],
	rules: {
	'import/default': 'off',
	'import/no-extraneous-dependencies': 'off',
	'import/no-unresolved': 'off',
	'import/named': 'off',
	'@wordpress/data-no-store-string-literals': 'off',
	'react-compiler/react-compiler': 'off',
	},
	},

[manzoorwanijk]

Yeah, may be we should do that.

Also, once we have the isolated dependencies via #75814, then we won't even need that rule anywhere.

[aduth]

Also, once we have the isolated dependencies via #75814, then we won't even need that rule anywhere.

Just to be clear before I go through the effort, will it be a necessary pre-step to resolve those existing issues where we rely on devDependencies in the root?

I just glanced at what happens when removing the import/no-extraneous-dependencies exception for these files, and it's 882 errors 🙈 We'll have to think of a way to break this down more incrementally, or maybe we can just rip off the bandaid and do it in one go.

I'll plan to create an issue unless it's not something that'll be beneficial for #75814.

[manzoorwanijk]

Just to be clear before I go through the effort, will it be a necessary pre-step to resolve those existing issues where we rely on devDependencies in the root?

By “existing issues”, if you mean those 882 errors, then no, we don’t need to fix those.

[aduth]

Just to be clear before I go through the effort, will it be a necessary pre-step to resolve those existing issues where we rely on devDependencies in the root?

By “existing issues”, if you mean those 882 errors, then no, we don’t need to fix those.

I'll check again in the morning, but my assumption was that those 882 lint errors are pointing to legitimate issues that would start breaking with #76195: i.e. a package implicitly depending on something coming from the root package.json. Less about ESLint errors themselves.

[manzoorwanijk]

When we adopt the isolated dependencies strategy and then complete #75041, we will not need that ESLint rule at all, including for the non-dev files.

The reason for #75041 is, even with isolated dependencies, (even with pnpm), every workspace can access the dependencies of any parent worspace (e.g. a monorepo root), because of Node's upward traversal algorithm. So, it's important to combine isolated dependencies with a clean root package.json.

When we have that, no workspace can access dependencies of other workspaces or of the root, thus forcing us to declare every dependency a workspace needs, which removes the need for a linter doing that job.