fix(content-header): allow button label text to wrap on long text and…#2582
fix(content-header): allow button label text to wrap on long text and…#2582SaiYugandhar03 wants to merge 12 commits into
Conversation
|
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a responsive secondary-actions slot to the ix-content-header component, allowing secondary action buttons to collapse into an overflow dropdown menu on smaller viewports. It also updates various preview examples and test files to utilize this slot, and introduces customizable CSS variables to the button mixin for more flexible styling. Feedback on these changes highlights a potential ReferenceError during Server-Side Rendering (SSR) due to direct window access, and suggests handling component re-attachment lifecycle events more robustly. Additionally, it is recommended to avoid hardcoding a restrictive fixed width on secondary action buttons in production CSS and to generalize the slotted selector to support other button-like components.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ::slotted(ix-button[slot='secondary-actions']) { | ||
| width: 5rem; | ||
| min-width: 5rem; | ||
| max-width: 5rem; | ||
|
|
||
| --ix-button-height: auto; | ||
| --ix-button-text-white-space: normal; | ||
| --ix-button-word-break: break-word; | ||
| --ix-button-inner-overflow: visible; | ||
| --ix-button-content-overflow: visible; | ||
| } |
There was a problem hiding this comment.
Forcing a fixed width of 5rem (80px) on all secondary action buttons in production CSS is extremely restrictive and will cause standard button labels (e.g., 'Settings', 'Export Data') to wrap awkwardly or truncate. If a fixed width is needed for visual regression testing, it should be applied directly in the test files (e.g., using inline styles or a test-specific class) rather than hardcoded in the component's production stylesheet. Additionally, using ::slotted([slot='secondary-actions']) instead of ::slotted(ix-button[slot='secondary-actions']) ensures that these wrapping and layout styles are also correctly applied to other button-like components (such as ix-icon-button) used in the preview examples.
::slotted([slot='secondary-actions']) {
--ix-button-height: auto;
--ix-button-text-white-space: normal;
--ix-button-word-break: break-word;
--ix-button-inner-overflow: visible;
--ix-button-content-overflow: visible;
}
SaiYugandhar03
force-pushed
the
Content-Header-Buttons-Wrap-into-Overflow-Menu
branch
from
4c862c1 to
8ff3402
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds responsive secondary-actions support to ChangesResponsive Secondary Actions Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/content-header/content-header.scss`:
- Around line 24-25: Remove the deprecated declaration `word-break: break-word`
in content-header.scss (the rule that currently also has `overflow-wrap:
break-word`) and either delete it entirely so `overflow-wrap: break-word` is the
sole wrapping rule, or if you really need aggressive intraword breaks use
`word-break: break-all`; update the CSS rule containing `overflow-wrap:
break-word` accordingly.
In `@packages/core/src/components/content-header/content-header.tsx`:
- Line 68: Extract the duplicated media query string into a single private
constant (e.g., MEDIA_QUERY = '(max-width: 48em)') inside the ContentHeader
class and replace the two inline uses in componentWillLoad and connectedCallback
where this.mediaQuery is assigned with that constant; update any references to
the literal string to use the constant to improve maintainability.
- Line 10: Add a new changeset markdown file under .changeset that covers the
ContentHeader responsive secondary actions + dropdown pattern: set the scope to
"`@siemens/ix`", pick the appropriate release type (minor for new non-breaking
feature, major if it changes behavior), and write a short summary describing the
responsive + dropdown behavior for ContentHeader (mention "responsive secondary
actions" and "dropdown pattern"); if the change is breaking, include explicit
migration guidance for consumers (what to update in ContentHeader usage and
props). Ensure the changeset references the
content-header/responsive/dropdown/secondary actions change so it will trigger a
release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59bf6654-fa71-4323-bd5f-248cf797c45e
⛔ Files ignored due to path filters (4)
testing/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-basic-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-basic-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-secondary-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-secondary-1-chromium---classic-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (21)
packages/angular-standalone-test-app/src/preview-examples/content-header-no-back.htmlpackages/angular-standalone-test-app/src/preview-examples/content-header-with-slot.htmlpackages/angular-standalone-test-app/src/preview-examples/content-header.htmlpackages/angular-test-app/src/preview-examples/content-header-no-back.htmlpackages/angular-test-app/src/preview-examples/content-header-with-slot.htmlpackages/angular-test-app/src/preview-examples/content-header.htmlpackages/core/src/components/button/button-mixin.scsspackages/core/src/components/content-header/content-header.scsspackages/core/src/components/content-header/content-header.tsxpackages/html-test-app/src/preview-examples/content-header-no-back.htmlpackages/html-test-app/src/preview-examples/content-header-with-slot.htmlpackages/html-test-app/src/preview-examples/content-header.htmlpackages/react-test-app/src/preview-examples/content-header-no-back.tsxpackages/react-test-app/src/preview-examples/content-header-with-slot.tsxpackages/react-test-app/src/preview-examples/content-header.tsxpackages/vue-test-app/src/preview-examples/content-header-no-back.vuepackages/vue-test-app/src/preview-examples/content-header-with-slot.vuepackages/vue-test-app/src/preview-examples/content-header.vuetesting/visual-testing/tests/content-header/basic/index.htmltesting/visual-testing/tests/content-header/secondary/index.htmltesting/visual-testing/tests/content/basic/index.html
|
|
||
| import { iconArrowLeft } from '@siemens/ix-icons/icons'; | ||
| import { Component, Event, EventEmitter, h, Host, Prop } from '@stencil/core'; | ||
| import { iconArrowLeft, iconMoreMenu } from '@siemens/ix-icons/icons'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for changeset files related to content-header
fd -e md . .changeset/ --exec grep -l 'content-header' {} \; || echo "No changeset found for content-header"Repository: siemens/ix
Length of output: 92
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .changeset files (names) =="
ls -1 .changeset 2>/dev/null || echo "(no .changeset dir)"
echo
echo "== Search for content-header / ContentHeader in .changeset =="
rg -n --no-heading "content-header|ContentHeader" .changeset || echo "No matches."
echo
echo "== Search for likely keywords in .changeset =="
rg -n --no-heading "dropdown|responsive|secondary actions|secondary actions|more menu|small viewport" .changeset || echo "No matches."Repository: siemens/ix
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .changeset markdown files =="
fd -e md . .changeset --exec basename {} \; | sort | uniq
echo
echo "== Search all changesets for broader terms =="
for pat in "content" "header" "dropdown" "responsive" "secondary" "menu"; do
echo "--- pattern: $pat ---"
rg -n --no-heading "$pat" .changeset --glob '*.md' || echo "No matches for $pat"
echo
doneRepository: siemens/ix
Length of output: 708
Add a changeset for the ContentHeader responsive + dropdown behavior
No .changeset/*.md entry exists for content-header/responsive/dropdown/secondary actions (only an unrelated modal-header-long-title changeset is present). Add a changeset in .changeset/ with scope @siemens/ix, the appropriate release type, and a summary of the new responsive secondary actions + dropdown pattern. Include migration guidance if this is a breaking behavior change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/content-header/content-header.tsx` at line 10,
Add a new changeset markdown file under .changeset that covers the ContentHeader
responsive secondary actions + dropdown pattern: set the scope to "`@siemens/ix`",
pick the appropriate release type (minor for new non-breaking feature, major if
it changes behavior), and write a short summary describing the responsive +
dropdown behavior for ContentHeader (mention "responsive secondary actions" and
"dropdown pattern"); if the change is breaking, include explicit migration
guidance for consumers (what to update in ContentHeader usage and props). Ensure
the changeset references the content-header/responsive/dropdown/secondary
actions change so it will trigger a release.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/button/button-mixin.scss (1)
113-137:⚠️ Potential issue | 🟠 MajorChangeset exists but may inadequately document the CSS custom property additions.
A changeset (
button-padding-css-variable.md) is present, but its description does not explicitly document the new CSS custom property contracts (--ix-button-content-overflow,--ix-button-text-white-space, etc.) being introduced tobutton-mixin.scss. These properties enable user-facing behavior changes—specifically, allowing buttons incontent-header's secondary-actions slot to wrap and overflow text instead of truncating. Please update the changeset summary to explicitly list the new CSS custom properties and their purpose.Regarding accessibility: extensive button axe-based tests exist, but no evidence of new tests for the content-header secondary-actions overflow behavior. Confirm whether accessibility coverage has been added for this specific UI change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/button/button-mixin.scss` around lines 113 - 137, Update the changeset file to explicitly document the new CSS custom properties being introduced in button-mixin.scss: --ix-button-content-overflow, --ix-button-text-white-space, --ix-button-content-text-overflow, and --ix-button-word-break. Include a clear description explaining that these properties enable buttons in content-header secondary-actions slots to wrap and overflow text instead of truncating. Additionally, verify that accessibility tests have been added for this specific content-header secondary-actions overflow behavior to ensure proper axe-based coverage for the UI change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/button/button-mixin.scss`:
- Line 104: Remove the empty line that appears before the padding property
declaration in the button-mixin.scss file. The `declaration-empty-line-before`
stylelint rule is being violated by the blank line preceding the `padding:
var(--ix-button-padding, 0 0.5rem);` declaration. Delete this empty line to
ensure the property declaration immediately follows the preceding content
without any blank lines in between.
---
Outside diff comments:
In `@packages/core/src/components/button/button-mixin.scss`:
- Around line 113-137: Update the changeset file to explicitly document the new
CSS custom properties being introduced in button-mixin.scss:
--ix-button-content-overflow, --ix-button-text-white-space,
--ix-button-content-text-overflow, and --ix-button-word-break. Include a clear
description explaining that these properties enable buttons in content-header
secondary-actions slots to wrap and overflow text instead of truncating.
Additionally, verify that accessibility tests have been added for this specific
content-header secondary-actions overflow behavior to ensure proper axe-based
coverage for the UI change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3067632c-a856-4ead-856b-0cb69dd0abd3
📒 Files selected for processing (1)
packages/core/src/components/button/button-mixin.scss
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/content-header/content-header.tsx (1)
170-189: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd accessibility and Storybook coverage for the new responsive/overflow behavior.
This introduces a new user-visible variation (overflow dropdown on small viewports, slotted secondary actions). Per the component-level review requirements, add axe-based component tests for the changed behavior and a Storybook story demonstrating the small-viewport overflow +
secondary-actionsslot. PR objectives indicate unit tests, accessibility, and Storybook are not yet complete.As per path instructions: "Ensure accessibility coverage exists with axe-based component tests where behavior/UI changed" and "Require a Storybook story for any new component or significant user-visible variation (states, themes, interactions)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/content-header/content-header.tsx` around lines 170 - 189, The responsive behavior in the content-header component that conditionally renders the `ix-dropdown-button` with secondary actions based on the `isSmallBreakpoint` property requires accessibility and Storybook coverage. Add axe-based component tests that verify accessibility compliance for both the small breakpoint path (with the dropdown) and the regular breakpoint path, ensuring the `secondary-actions` slot is properly accessible in both scenarios. Additionally, create a Storybook story that demonstrates the small viewport overflow behavior by setting `isSmallBreakpoint` to true and showcasing the `secondary-actions` slot populated with dropdown content.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/content-header/content-header.tsx`:
- Around line 102-112: The slotchange listener in componentDidLoad uses an
inline arrow function that cannot be removed, causing a leak, and is only
registered once so it is lost after element reconnect. Create a stable handler
reference as a class property (similar to how mediaQueryHandler is handled),
attach the slotchange listener to the secondary-actions slot in both
componentDidLoad and connectedCallback to ensure it is re-registered after
disconnect/reconnect, and remove the listener in disconnectedCallback using the
stored handler reference.
- Around line 174-182: The ix-dropdown-button component rendering the overflow
menu icon has an empty label attribute, which prevents screen readers from
announcing the button's purpose. Replace the empty label="" with a meaningful,
localizable string such as "More actions" to provide an accessible name that
describes the dropdown's function to assistive technology users.
- Around line 73-92: The checkSecondarySlot method has a chicken-and-egg problem
where it queries a conditionally-rendered shadow DOM slot that only appears when
hasSecondaryActions is true, preventing initial detection on small viewports.
Instead of querying the shadow DOM slot element using querySelector on
slot[name="secondary-actions"], query the host element's light-DOM children
directly by selecting elements with the slot="secondary-actions" attribute using
this.hostElement.querySelectorAll('[slot="secondary-actions"]'). Filter the
results for meaningful nodes (excluding empty text nodes) and set
hasSecondaryActions based on whether any meaningful content exists, making the
detection independent of render state.
---
Outside diff comments:
In `@packages/core/src/components/content-header/content-header.tsx`:
- Around line 170-189: The responsive behavior in the content-header component
that conditionally renders the `ix-dropdown-button` with secondary actions based
on the `isSmallBreakpoint` property requires accessibility and Storybook
coverage. Add axe-based component tests that verify accessibility compliance for
both the small breakpoint path (with the dropdown) and the regular breakpoint
path, ensuring the `secondary-actions` slot is properly accessible in both
scenarios. Additionally, create a Storybook story that demonstrates the small
viewport overflow behavior by setting `isSmallBreakpoint` to true and showcasing
the `secondary-actions` slot populated with dropdown content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a7a5d68-d074-4570-8b6f-9f20aedaa89f
⛔ Files ignored due to path filters (4)
testing/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-basic-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-basic-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-secondary-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/content-header/content-header.e2e.ts/content-header-secondary-1-chromium---classic-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
packages/core/src/components/content-header/content-header.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/components/content-header/content-header.tsx (1)
178-196: 📐 Maintainability & Code Quality | 🟡 MinorAdd small-breakpoint coverage for
ix-content-header
testing/framework-tests/tests/generated/content-header-axe.spec.tsonly scans the default preview, andtesting/framework-tests/tests/working-with-axe.spec.ts/testing/framework-tests/tests/working.spec.tsdo not include anycontent-headercases. Add a viewport-sized overflow case that opens the secondary-actions menu and wire it into both accessibility suites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/content-header/content-header.tsx` around lines 178 - 196, Add a small-breakpoint test case for ix-content-header that exercises the overflow menu by rendering the component with secondary-actions, switching to a narrow viewport, and opening the ix-dropdown-button from the content-header actions area. Then wire that same scenario into both the generated axe suite and the working/working-with-axe suites so content-header is covered beyond the default preview; use the existing content-header test helpers/spec entries as the place to register the new case.Source: Path instructions
♻️ Duplicate comments (1)
packages/core/src/components/content-header/content-header.tsx (1)
182-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProvide a non-empty accessible name for the overflow trigger.
ix-dropdown-buttonis icon-only withlabel="", so assistive tech gets no meaningful name.♿ Suggested fix
<ix-dropdown-button icon={iconMoreMenu} variant="tertiary" - label="" + label="More actions" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/components/content-header/content-header.tsx` around lines 182 - 186, The overflow trigger in the content header is icon-only and currently has an empty accessible name via ix-dropdown-button’s label prop. Update the ContentHeader overflow button so it exposes a non-empty, meaningful label (or equivalent accessible name) that describes the menu action, using the existing ix-dropdown-button usage in ContentHeader to locate it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/core/src/components/content-header/content-header.tsx`:
- Around line 178-196: Add a small-breakpoint test case for ix-content-header
that exercises the overflow menu by rendering the component with
secondary-actions, switching to a narrow viewport, and opening the
ix-dropdown-button from the content-header actions area. Then wire that same
scenario into both the generated axe suite and the working/working-with-axe
suites so content-header is covered beyond the default preview; use the existing
content-header test helpers/spec entries as the place to register the new case.
---
Duplicate comments:
In `@packages/core/src/components/content-header/content-header.tsx`:
- Around line 182-186: The overflow trigger in the content header is icon-only
and currently has an empty accessible name via ix-dropdown-button’s label prop.
Update the ContentHeader overflow button so it exposes a non-empty, meaningful
label (or equivalent accessible name) that describes the menu action, using the
existing ix-dropdown-button usage in ContentHeader to locate it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e081f7d2-933a-47cd-bc1e-77a913878c5f
📒 Files selected for processing (2)
packages/core/src/components/button/button-mixin.scsspackages/core/src/components/content-header/content-header.tsx
💤 Files with no reviewable changes (1)
- packages/core/src/components/button/button-mixin.scss
|
1 participant
… small viewports
💡 What is the current behavior?
GitHub Issue Number: #2125
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
ix-content-headernow detects secondary actions and displays them responsively: on small screens, they move into an overflow “more” menu.secondary-actionsslot.