Admin UI: Add icon prop to Page header component

[CGastrell]

See design convo #76709

What?

Adds a icon 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 icon prop renders the logo outside the h2 tag, ensuring:

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

How?

• Added icon?: 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 <cgastrell@git.wordpress.org>
Co-authored-by: simison <simison@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: aduth <aduth@git.wordpress.org>

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.

[jameskoster]

Looking at the spec (#76709) it seems icons and images are to be treated the same way, so Visual seems like a good option to align with 👍

[CGastrell]

Looking at the spec (#76709) it seems icons and images are to be treated the same way, so Visual seems like a good option to align with 👍

Just to confirm, the prop should be renamed from icon to visual? As in:

<Page title="Page title" visual={ <Icon icon={ wordpress } size={ 24 } /> }>...</Page>

[jameskoster]

That makes more sense to me, given it can render things that are not icons.

[CGastrell]

@jameskoster I pushed the rename from icon to visual, looking good :)

[jameskoster]

Tiny detail, but this kind of looks like an icon when rendered, while the story is supposed to demonstrate using an image. Is there an alternative placeholder we could use? Matt's gravatar or something?

[CGastrell]

@jameskoster

I switched the sample image for Matt's gravatar. Then addressed the other comments, they all made sense to me.

[jameskoster]

Seems good from a design perspective 👍

[CGastrell]

Seems good from a design perspective 👍

@ciampo @simison any particular thoughts from engineering side?

[simison]

Let's include a changelog entry for admin-ui.

[CGastrell]

Let's include a changelog entry for admin-ui.

Added on 925cd17

Thanks for pointing it out!

[ciampo]

Although I'd like to rethink the whole Page abstraction holistically, adding the visual prop can work as a short-term addition to unlock consumption of the component

[ciampo]

Any reason in particular why we're targeting img and svg elements? What if a different tag (eg figure, video, etc) is passed?

These styles may be unnecessary and introduce a wrong abstraction and/or a style leak: IMO, it's the consumer who's passing visual who should ensure the visual element that is being passed is styled correctly.

If we want to control the layout a bit more intentionally, we could potentially use CSS grid? ie. a 1x1 grid, forcing all children to occupy the grid cell;

.admin-ui-page__header-visual { 
  display: grid;
  grid-template-columns: 1fr;
  grid-template-rows: 1fr;
  width: 24px;
  height: 24px;

  > * {
    grid-column: 1 / -1;
    grid-row: 1 / -1;
  }
}