EuiHeaderLogo new logoType prop - #9875
Conversation
10 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiheader (8 differences)
euicollapsiblenav (2 differences)
|
14 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiheader (3 differences)
euidatagrid (11 differences)
|
14 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiheader (3 differences)
euidatagrid (11 differences)
|
13 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiheader (3 differences)
euidatagrid (10 differences)
|
|
Hey @weronikaolejniczak I'd like to get your help on this. I decided to conditionally render either an EuiIcon or embedded SVG to display the glyph logo or the full logo. {logoType === 'glyph' && (
<EuiIcon
aria-label={iconTitle}
className="euiHeaderLogo__icon"
size="l"
type={iconType}
/>
)}
{logoType === 'horizontal' && (
<SvgElasticLogoFull
title={iconTitle}
style={{ maxHeight: euiTheme.size.l, width: 'auto' }}
className="euiHeaderLogo__image"
/>
)}The EuiIcon constrains the size to always a box container. And it changes based on the theme applied as shown here: 
But when we force the EuiHeader with theme={"dark"} looks like it doesn't affect the EuiHeraderLogo content and doesn't force it to behave like always dark as I'd expect: 
|
|
@JoseLuisGJ we'd need to make the SVG use .euiHeaderLogo__text,
.euiHeaderLogo__image, // <- add this
.euiHeaderLink,
.euiHeaderSectionItemButton {
color: ${euiTheme.colors.textGhost};
}The header deliberately doesn't use |
|
@JoseLuisGJ we'd need to make the SVG use .euiHeaderLogo__text,
.euiHeaderLogo__image, // <- add this
.euiHeaderLink,
.euiHeaderSectionItemButton {
color: ${euiTheme.colors.textGhost};
}The header deliberately doesn't use |
|
Thanks @weronikaolejniczak now it also applies properly the theme dark to the header in Docs: 
|
There was a problem hiding this comment.
Pull request overview
This PR updates EuiHeaderLogo to support a new logoType prop that switches between the existing Elastic glyph and a new inline horizontal full-wordmark SVG, and updates EUI docs/stories/tests to reflect the new usage.
Changes:
- Added a new
logoType?: 'glyph' | 'horizontal'API and horizontal SVG render path forEuiHeaderLogo - Introduced an inline
SvgElasticLogoFullcomponent and updated styling for dark headers - Updated Storybook stories, a11y example, docs examples, and Jest snapshots for the new output
Reviewed changes
Copilot reviewed 12 out of 63 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/website/docs/components/layout/header.mdx | Updates documentation examples to use logoType="horizontal" and removes iconType/children usage. |
| packages/eui/src/components/header/header.styles.ts | Ensures dark header styling applies to the new horizontal logo element class. |
| packages/eui/src/components/header/header.stories.tsx | Updates header stories to stop passing iconType/children and demonstrate horizontal logo usage. |
| packages/eui/src/components/header/header.a11y.tsx | Updates the header a11y example to no longer pass iconType. |
| packages/eui/src/components/header/header_logo/header_logo.tsx | Implements the logoType switch and renders the new horizontal SVG variant. |
| packages/eui/src/components/header/header_logo/header_logo.test.tsx | Updates unit tests to reflect new rendering behavior and adds an iconType legacy assertion. |
| packages/eui/src/components/header/header_logo/header_logo.styles.ts | Removes text styling now that text children rendering is removed. |
| packages/eui/src/components/header/header_logo/header_logo.stories.tsx | Updates stories to demo glyph vs horizontal variants. |
| packages/eui/src/components/header/header_logo/elastic_logo_full.tsx | Adds the new inline horizontal full-wordmark SVG component. |
| packages/eui/src/components/header/header_logo/snapshots/header_logo.test.tsx.snap | Updates snapshots for the new rendered markup (now includes the inline SVG). |
| packages/eui/src/components/collapsible_nav/collapsible_nav.stories.tsx | Updates an example to use the horizontal logo variant. |
| packages/eui/changelogs/upcoming/9875.md | Adds a changelog entry describing the new logoType prop. |
Suppressed comments (1)
packages/eui/src/components/header/header_logo/header_logo.test.tsx:56
renders optional propsnow snapshots the entire inline horizontal SVG, which makes the snapshot very large and brittle (minor SVG output changes will cause big diffs). Consider replacing the snapshot with targeted assertions (e.g..euiHeaderLogo__imageexists, a<title>containsiconTitle, and styles are applied), and deleting/updating the corresponding snapshot entry.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
44 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiaspectratio (1 difference)
euilistgroup (2 differences)
euititle (1 difference)
euiexpression (3 differences)
euiform (7 differences)
euisuperdatepicker (9 differences)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiheader (4 differences)
euipage (1 difference)
euicontextmenu (3 differences)
euidatagrid (6 differences)
euitable (1 difference)
euithemeprovider (4 differences)
|
44 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiaspectratio (1 difference)
euilistgroup (2 differences)
euititle (1 difference)
euiexpression (3 differences)
euiform (7 differences)
euisuperdatepicker (9 differences)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiheader (4 differences)
euipage (1 difference)
euicontextmenu (3 differences)
euidatagrid (6 differences)
euitable (1 difference)
euithemeprovider (4 differences)
|
44 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiaspectratio (1 difference)
euilistgroup (2 differences)
euititle (1 difference)
euiexpression (3 differences)
euiform (7 differences)
euisuperdatepicker (9 differences)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiheader (4 differences)
euipage (1 difference)
euicontextmenu (3 differences)
euidatagrid (6 differences)
euitable (1 difference)
euithemeprovider (4 differences)
|
45 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiaspectratio (1 difference)
euilistgroup (2 differences)
euititle (1 difference)
euiexpression (3 differences)
euiform (7 differences)
euisuperdatepicker (9 differences)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiheader (4 differences)
euipage (1 difference)
euicontextmenu (3 differences)
euidatagrid (7 differences)
euitable (1 difference)
euithemeprovider (4 differences)
|
92 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiaspectratio (1 difference)
euibadge (5 differences)
euicallout (9 differences)
euilistgroup (2 differences)
euititle (1 difference)
euitour (1 difference)
euicombobox (1 difference)
euiexpression (3 differences)
euifiltergroup (3 differences)
euiform (8 differences)
euirange (7 differences)
euisuperdatepicker (9 differences)
euilistitemlayout (10 differences)
euibottombar (1 difference)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiheader (4 differences)
euiinputpopover (3 differences)
euipage (1 difference)
euicontextmenu (3 differences)
euisidenav (2 differences)
euidatagrid (6 differences)
euiinmemorytable (2 differences)
euitable (1 difference)
euiprovider (2 differences)
euithemeprovider (4 differences)
|
|
@JoseLuisGJ could you rebase onto main? This should fix the weird VRTs. |
44 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiaspectratio (1 difference)
euilistgroup (2 differences)
euititle (1 difference)
euiexpression (3 differences)
euiform (7 differences)
euisuperdatepicker (9 differences)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiheader (4 differences)
euipage (1 difference)
euicontextmenu (3 differences)
euidatagrid (6 differences)
euitable (1 difference)
euithemeprovider (4 differences)
|
54bd25e to
3282530
Compare
3282530 to
58a4628
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/eui/src/components/header/header_logo/header_logo.tsx:28
- Removing
iconTypefrom the public props leaves an existing repository usage atpackages/website/docs/components/display/progress.mdx:181-185passingiconType="logoKibana". That example now uses an invalid prop and, at runtime, silently renders the Elastic glyph instead of the Kibana logo. Update that example as part of this API migration.
export type EuiHeaderLogoProps = CommonProps &
Omit<AnchorHTMLAttributes<HTMLAnchorElement>, 'children'> & {
href?: string;
packages/eui/src/components/header/header_logo/header_logo.stories.tsx:41
- This story still appears in Storybook as “With Text”, although the changed API removes text children and the story now demonstrates the horizontal logo. Rename it to
Horizontalso the story does not imply that text content remains supported.
args: {
logoType: 'horizontal',
packages/eui/changelogs/upcoming/9875.md:1
- This entry is formatted only as an addition and omits the removal of
iconType, even though this PR removes bothiconTypeandchildren. The changelog template records removals under**Breaking changes**(packages/eui/changelogs/upcoming/_template.md:11-13); document both removals there so consumers receive complete migration information.
- Added new `logoType` prop to the `EuiHeaderLogo` component to render either the `glyph` or `horizontal` Elastic logo versions. `children` text content is no longer rendered.
|
Copilot's suppressed comments in: #9875 (review) are actually worth tackling |
65 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuicomment (2 differences)
euidroppable (2 differences)
euipinnablelistgroup (1 difference)
euitimeline (2 differences)
euititle (1 difference)
euicolorpicker (4 differences)
euiform (2 differences)
euiradiogroup (2 differences)
euiselectable (4 differences)
euisuperdatepicker (9 differences)
euiconfirmmodal (1 difference)
euiflexitem (1 difference)
euiflyout (1 difference)
euiheader (11 differences)
euiinputpopover (3 differences)
euipopover (4 differences)
euicollapsiblenav (3 differences)
euisteps (2 differences)
euidatagrid (8 differences)
euiautosizer (1 difference)
euioutsideclickdetector (1 difference)
|
weronikaolejniczak
left a comment
weronikaolejniczak
left a comment
There was a problem hiding this comment.
LGTM 🟢 Thanks, Jose 🙏🏻 Let's make those VRT pass and we can merge.
| background-color: ${backgroundColor}; | ||
|
|
||
| .euiHeaderLogo, | ||
| .euiHeaderLogo__text, |
There was a problem hiding this comment.
nit:
| .euiHeaderLogo__text, |
I don't think this is needed anymore, right?
| // Keep this to prevent legacy runtime `iconType` props from being forwarded to the anchor. | ||
| const { iconType: _ignoredIconType, ...anchorProps } = rest as typeof rest & { | ||
| iconType?: unknown; | ||
| }; |
There was a problem hiding this comment.
nit:
A more TS-idiomatic way would be adding iconType?: never to EuiHeaderLogoProps which would block it at compile time.
| {logoType === 'horizontal' && ( | ||
| <SvgElasticLogoFull | ||
| title={iconTitle} | ||
| style={{ maxHeight: euiTheme.size.l, width: 'auto' }} |
There was a problem hiding this comment.
nit:
This style can be added to euiHeaderLogo__image instead of inline, no?
weronikaolejniczak
left a comment
weronikaolejniczak
left a comment
There was a problem hiding this comment.
Actually, I checked the Kibana usage and this is a breaking change. We should add a "breaking change" label and prepare appropriate changes, add commits both in Kibana and Cloud UI to the PR description and preferably also update kibana-prep-commits file with that commit.
12 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuiheader (8 differences)
euicollapsiblenav (2 differences)
euidatagrid (2 differences)
|
I found in Kibana its usage into the: |
4 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuidatagrid (4 differences)
|
JoseLuisGJ
added
the
breaking change
|
This PR contains breaking changes. The opener of this pull request is asked to perform the following due diligence steps below, to assist EUI in our next Kibana upgrade:
|
| @@ -0,0 +1 @@ | |||
| - Added new `logoType` prop to the `EuiHeaderLogo` component to render either the `glyph` or `horizontal` Elastic logo versions. `children` text content is no longer rendered. No newline at end of file | |||
There was a problem hiding this comment.
blocking:
We need to mark this as Breaking changes:
| - Added new `logoType` prop to the `EuiHeaderLogo` component to render either the `glyph` or `horizontal` Elastic logo versions. `children` text content is no longer rendered. | |
| **Breaking changes** | |
| - Added new `logoType` prop to the `EuiHeaderLogo` component to render either the `glyph` or `horizontal` Elastic logo versions. `children` text content is no longer rendered. |
And we need to prepare commits for both Kibana and Cloud UI that use the children API.
Cloud UI:
cloud-ui/public/components/ChromeHeader/HeaderNavigation/index.tsx
We need to update it to uselogoType="horizontal", drop the children/iconType/ElasticMarkCSS. Also, probably drop:cloud-ui/public/components/ChromeHeader/HeaderNavigation/ElasticMark.tsxcloud-ui/public/components/ChromeHeader/HeaderNavigation/HeaderNavigation.test.tsx
AutoOps:
autoops/src/shared/layouts/TopMenu/TopMenu.tsx
Kibana:
src/platform/kbn-ui/chrome-layout/packaging/example/src/app.tsx
Also, the new Kibana Chrome uses a custom logo. It's worth bringing up to the team if we should reuse EuiHeaderLogo there. Otherwise, it'll become redundant.
src/core/packages/chrome/browser-components/src/classic/header_logo.tsxsrc/core/packages/chrome/browser-components/src/chrome_next/global_header/global_header_logo.tsx
weronikaolejniczak
left a comment
weronikaolejniczak
left a comment
There was a problem hiding this comment.
progress.mdx still passes iconType="logoKibana".
We'd also need to resolve the conflict and rebase onto latest upstream main.
💚 Build Succeeded
History
|
3 visual difference(s) found - expand to review, then click Approve visual changes to update baselineseuidatagrid (3 differences)
|
💔 Build Failed
Failed CI StepsHistory
|
Summary
EuiHeaderLogoto support a newlogoTypeAPI that switches between the Elastic glyph logo and a new horizontal full-wordmark logo. Added a dedicated inline SVG component for the horizontal logo and updated stories/tests/snapshots accordingly.logoType?: 'glyph' | 'horizontal'toEuiHeaderLogoin sync with the naming convention if Elastic branding.SvgElasticLogoFullinline SVG and render path whenlogoType="horizontal".API Changes
glyph(default) orhorizontal.Screenshots
logoType="glyph"orlogoType="horizontal"Docs
Impact Assessment
Note: Most PRs should be tested in Kibana to help gauge their Impact before merging.
iconTypeandchildrenwere removed fromEuiHeaderLogo. Any consumer using custom icon types or inline text content will need updates.Impact level: 🟡 Moderate
Release Readiness
EuiHeaderLogonewlogoTypeprop and removediconType/children.iconTypeusage withlogoTypewhere applicable.EuiHeaderLogo.logoType="horizontal"where full wordmark is desired.QA instructions for reviewer
logoType="glyph"still renders expected Elastic glyph size and alignment.logoType="horizontal"renders the new full logo at expected dimensions.iconTitleis announced correctly in glyph and horizontal modes.href,rel,target, secure rel behavior.Checklist before marking Ready for Review
breaking changelabel (if applicable)Reviewer checklist