fix(core): render semantic headings for application and content headers - #2703
lakshmi-priya-b wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 679d77d The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughApplication and content headers now use native ChangesHeader semantics and typography
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The header titles now use native headings while retaining their visual variants and omission behavior. No actionable merge risk remains from the supplied evidence. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🤖 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/application-header/application-header.scss`:
- Around line 106-108: Add the required empty line after the typography mixin
and before margin in
packages/core/src/components/application-header/application-header.scss (lines
106-108). In packages/core/src/components/content-header/content-header.scss
(lines 31-40), add empty lines before margin and the secondary padding
declaration to satisfy Stylelint.
In `@packages/core/src/components/application-header/application-header.tsx`:
- Line 409: Conditionally render the h1.application-name in the
application-header component only when this.name has content, and conditionally
render the h2.header-title in content-header only when this.headerTitle has
content; update regression coverage for omitted titles, preserving accessible
heading behavior for provided titles. Affected sites:
packages/core/src/components/application-header/application-header.tsx lines
409-409 requires the conditional h1 change;
packages/core/src/components/content-header/content-header.tsx lines 63-71
requires the conditional h2 change.
- Line 409: Add a package-scoped changeset describing the consumer-facing
heading-semantic and accessibility changes for ix-application-header at
packages/core/src/components/application-header/application-header.tsx:409-409
and ix-content-header at
packages/core/src/components/content-header/content-header.tsx:63-71; do not
classify the change as internal-only unless that justification is explicitly
valid.
In
`@packages/core/src/components/application-header/test/application-header.ct.ts`:
- Around line 14-33: Replace the plain test calls in the accessibility and
application-name heading tests with regressionTest, and import regressionTest
from `@utils/test`. Preserve the existing test bodies and assertions.
- Around line 23-32: Update the heading assertions in
packages/core/src/components/application-header/test/application-header.ct.ts
(lines 23-32) and
packages/core/src/components/content-header/test/content-header.ct.ts (lines
48-55) to use accessibility locators: getByRole('heading', { level: 1, name:
'Test Application' }) and getByRole('heading', { level: 2, name: 'My Content
Page' }). Retain the secondary variant’s existing class assertion.
In `@packages/core/src/components/content-header/content-header.scss`:
- Around line 36-39: In the .secondary rule of the content-header styles, import
misc/common-variables under the vars namespace and replace the hardcoded 0.25rem
padding value with vars.$tiny-space, preserving the existing vertical padding
behavior.
🪄 Autofix
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 Plus
Run ID: 39ed5d6b-35f5-4166-bdd2-9138a8341b69
📒 Files selected for processing (6)
packages/core/src/components/application-header/application-header.scsspackages/core/src/components/application-header/application-header.tsxpackages/core/src/components/application-header/test/application-header.ct.tspackages/core/src/components/content-header/content-header.scsspackages/core/src/components/content-header/content-header.tsxpackages/core/src/components/content-header/test/content-header.ct.ts
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/content-header/test/content-header.ct.ts (1)
31-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the primary variant’s class state.
The test data sets
hasSecondaryClass: false, but the assertion runs only fortrue. A regression that addssecondaryto the primary heading still passes. Assert that the primary heading does not have thesecondaryclass.Proposed fix
- if (hasSecondaryClass) { - await expect( - page.locator('ix-content-header').locator('h2.header-title') - ).toHaveClass(/\bsecondary\b/); - } + if (hasSecondaryClass) { + await expect(heading).toHaveClass(/\bsecondary\b/); + } else { + await expect(heading).not.toHaveClass(/\bsecondary\b/); + }🤖 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/test/content-header.ct.ts` around lines 31 - 58, Update the variant loop’s class assertion around hasSecondaryClass so both states are validated: retain the existing secondary-class assertion for the secondary variant and assert that the primary variant’s h2.header-title does not have the secondary class when hasSecondaryClass is false.
🤖 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 63-73: Add the title attribute to the h2 rendered in the
ContentHeader component, binding it to this.headerTitle so truncated titles
retain the existing full-title browser tooltip. Keep the current titleOverflow
class and other rendering behavior unchanged.
---
Outside diff comments:
In `@packages/core/src/components/content-header/test/content-header.ct.ts`:
- Around line 31-58: Update the variant loop’s class assertion around
hasSecondaryClass so both states are validated: retain the existing
secondary-class assertion for the secondary variant and assert that the primary
variant’s h2.header-title does not have the secondary class when
hasSecondaryClass is false.
🪄 Autofix
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 Plus
Run ID: 1d617ce5-e81e-4c5e-bbd9-cacf6da4a6ab
📒 Files selected for processing (6)
packages/core/src/components/application-header/application-header.scsspackages/core/src/components/application-header/application-header.tsxpackages/core/src/components/application-header/test/application-header.ct.tspackages/core/src/components/content-header/content-header.scsspackages/core/src/components/content-header/content-header.tsxpackages/core/src/components/content-header/test/content-header.ct.ts
|
2 participants
💡 What is the current behavior?
ix-application-header and ix-content-header render their titles through ix-typography, so the headings are not native HTML heading elements. The visual style depends on the typography format prop, and the DOM does not expose semantic h1/h2 structure.
JIRA : IX-4327
🆕 What is the new behavior?
ix-application-header now renders its title as a native h1, and ix-content-header renders its title as a native h2. The visual typography stays the same through SCSS mixins, and the variant-specific styling is preserved with classes instead of heading-format props.
🏁 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
<h1>headings.<h2>headings, improving document structure and assistive technology support.