-
Notifications
You must be signed in to change notification settings - Fork 860
Reduce EuiPageHeader tabs sizes for Borealis theme
#9049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
EuiPageHeader tabs sizes for Borealis theme
packages/eui/src/components/page/page_header/page_header_content.tsx
Outdated
Show resolved
Hide resolved
| !!responsive | ||
| ); | ||
|
|
||
| const euiTheme = useEuiTheme(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can restructure it here, right?
| const euiTheme = useEuiTheme(); | |
| const { euiTheme } = useEuiTheme(); |
|
I see I have conflicts on these visual regression updates... is that typical? |
|
Moving back to Draft; refactoring to use theme component tokens which proved to be much simpler in #9057 |
c26bb51 to
0032a35
Compare
That may be the case if the VRT images have been updated on main. In such a situation, I'd accept the incoming version and re-run |
packages/eui/src/components/page/page_header/page_header_content.tsx
Outdated
Show resolved
Hide resolved
|
Ok, addressed feedback, ready again! |
weronikaolejniczak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🟢 Thank you for the contribution and for the patience, @ryankeairns 🙏🏻
- Add _EuiThemePage interface and page theme extension to Borealis theme - Set pageHeader.tabsSize to 'm' for Borealis (reduced from default 'l') - Update EuiPageHeaderContent to use theme-based tabs size with fallback - Maintain backward compatibility - Amsterdam continues to use default 'l' - Update static JSON theme files to include page configuration - Fix interactive examples in docs to make tabs clickable - Update test snapshots to reflect new theming behavior This allows themes to customize page header tab sizes while maintaining sensible defaults for themes that don't specify custom values.
Update snapshots to reflect new theming behavior for tabs size.
- Updated EuiPageHeader and EuiPageHeaderContent baselines to reflect new Borealis theme tab size 'm' - Updated EuiPageTemplate baselines that use EuiPageHeader - Updated other component baselines from visual regression test run
- Add _EuiThemePageHeader type to eui-theme-common - Create pageHeader component tokens in Borealis theme (tabsSize: 'm') - Add default pageHeader tokens in Amsterdam theme (tabsSize: 'l') - Update EuiPageHeaderContent to use euiTheme.components.pageHeader.tabsSize - Export EuiTabsSizes type for proper TypeScript support - Update THEMING_TEMPLATE.md with real-world implementation lessons - Follow component tokens pattern for type safety and consistency Resolves circular dependency issues through proper type export chain and correct build order (eui-theme-common → eui-theme-borealis → eui)
- Remove type assertion and let TypeScript infer EuiTabsSizes from theme - Remove unused EuiTabsSizes import from page header component - Add proper type import and annotation to Amsterdam theme components
- Use explicit type annotation instead of type assertion - Fixes TypeScript error: Type 'string' is not assignable to type 'EuiTabsSizes' - Apply Prettier formatting fixes
b8f2911 to
6661a33
Compare
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
| "euiColorBorderStrongDanger": "#C61E25", | ||
| "euiColorBorderStrongText": "#5A6D8C" | ||
| "euiColorBorderStrongText": "#5A6D8C", | ||
| "page": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 I'm not sure that this is really that useful on the JSON export (it's not a value to use as a token).
That being said, if we add these for Borealis we should add them for Amsterdam as well (example file), otherwise switching themes would break if these are used somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I realize, we still have these files duplicated in packages/eui/src/themes/json and packages/eui-theme-borealis/src.
Originally it was duplicated when introducing Borealis as there were issues loading the file from the theme package. But that is not the case anymore and Kibana was updated (file) to use the eui-theme-borealis file.
That means this change is not actually available in Kibana as the packages/eui-theme-borealis/src files were not updated.
This reverts commit 7f2df0d.
Summary
Part of a series of adjustments to realize the Borealis vision and move away from the hallmarks of Amsterdam.
Why are we making this change?
Stylistic decisions for Amsterdam - like large, heavy titles and tabs - were baked into the components. This PR is one of several that bring additional theme override capabilities.
Screenshots
Before and after from docs examples; reduce tabs from 16 to 14
Looking ahead, this screenshot include forthcoming changes to also reduce title and text
Impact to users
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
@defaultif default values are missing) and playground toggles