Repository navigation
fix: (core/blind and core/push-card) Refactor: Align Border Coloring and Outline Variants with Design System - #2520
Conversation
🦋 Changeset detectedLatest commit: af3743d The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
![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 refactors the styling for the blind and push-card components, introducing a new header wrapper and specific styles for various variants like outline, alarm, and warning. The feedback identifies that a changeset is required for these user-facing changes and that axe-based accessibility tests should be added to cover the new interaction states. Additionally, the border-radius calculations in both components should be corrected to ensure visual symmetry between the top and bottom corners.
1307-Dev
changed the title
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
✅ 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:
📝 WalkthroughWalkthroughUpdates variant-specific SCSS for ChangesVariant Border & Animation Styling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/brave-jeans-rule.md (1)
1-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd release-closing issue linkage to the changeset summary.
This changeset is tied to tracked work but lacks
Fixes #<issue-number>in the summary, so release-time auto-closing will not happen per policy.As per coding guidelines, "Include
Fixes #<issue-number>syntax in changeset summary to automatically close linked GitHub issues when the changeset is released."Suggested update
--- "`@siemens/ix`": patch --- -Color **ix-blind** borders accorindg to variant. +Color **ix-blind** borders according to variant. +Fixes #<issue-number>🤖 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 @.changeset/brave-jeans-rule.md around lines 1 - 6, The changeset summary in the file is missing a `Fixes #<issue-number>` reference, which prevents the automatic closing of linked GitHub issues upon release. Add the `Fixes #<issue-number>` syntax to the changeset summary section (the text that describes the change related to the ix-blind borders and variant coloring) to ensure the associated GitHub issue is automatically closed when this changeset is released, following the coding guidelines for changeset documentation.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 @.changeset/brave-jeans-rule.md:
- Line 5: The changeset summary in the brave-jeans-rule.md file contains a typo
on line 5 where "accorindg" is misspelled. Correct this spelling error by
replacing "accorindg" with "according" in the summary text that reads "Color
**ix-blind** borders accorindg to variant."
---
Outside diff comments:
In @.changeset/brave-jeans-rule.md:
- Around line 1-6: The changeset summary in the file is missing a `Fixes
#<issue-number>` reference, which prevents the automatic closing of linked
GitHub issues upon release. Add the `Fixes #<issue-number>` syntax to the
changeset summary section (the text that describes the change related to the
ix-blind borders and variant coloring) to ensure the associated GitHub issue is
automatically closed when this changeset is released, following the coding
guidelines for changeset documentation.
🪄 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: 7befa459-804d-4ab0-ac50-134e14c503e2
⛔ Files ignored due to path filters (8)
testing/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-basic-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-basic-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-collapsed-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-collapsed-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-custom-header-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-custom-header-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/push-card/push-card.e2e.ts/push-card-basic-should-click-accordion-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/push-card/push-card.e2e.ts/push-card-basic-should-click-accordion-1-chromium---classic-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (5)
.changeset/brave-jeans-rule.md.changeset/brave-jeans-rulez.mdpackages/core/src/components/blind/blind.scsspackages/core/src/components/card-accordion/card-accordion.scsspackages/core/src/components/push-card/push-card.scss
1307-Dev
changed the title
nuke-ellington
left a comment
nuke-ellington
left a comment
There was a problem hiding this comment.
Collapsed outline has a 1px blind-content
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/components/blind/blind.scss (1)
18-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBorder token naming is inconsistent and may not match design spec.
alarmandwarninguse-bdrsuffixed tokens (--theme-color-alarm-bdr,--theme-color-warning-bdr), butcritical,info,success,neutral, andprimaryreference plain color tokens without a border-specific variant. This mirrors a previously flagged concern that border tokens don't fully match Jira/Figma. iX design tokens for borders typically use a dedicated*-bdrsuffix pattern across all status colors, so the missing suffix on 5 of 7 entries looks like an oversight rather than an intentional choice.🎨 Suggested consistent mapping (verify exact token names against Figma/theme package)
$blind-status-border: ( alarm: var(--theme-color-alarm-bdr), - critical: var(--theme-color-critical), + critical: var(--theme-color-critical-bdr), warning: var(--theme-color-warning-bdr), - info: var(--theme-color-info), + info: var(--theme-color-info-bdr), - success: var(--theme-color-success), + success: var(--theme-color-success-bdr), - neutral: var(--theme-color-neutral), + neutral: var(--theme-color-neutral-bdr), - primary: var(--theme-color-primary), + primary: var(--theme-color-primary-bdr), );#!/bin/bash # Check which "-bdr" token variants actually exist in the theme package rg -n '\-\-theme-color-(critical|info|success|neutral|primary)(-bdr)?\b' --type=scss --type=css rg -n '\-\-theme-color-.*-bdr' packages🤖 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/blind/blind.scss` around lines 18 - 26, The blind status border token map is inconsistent: `blind.scss` mixes border-specific `*-bdr` tokens with plain color tokens in `$blind-status-border`. Update the `$blind-status-border` entries in this map so all status borders use the correct border-specific theme token naming, and verify the exact token names against the theme package/design spec for `critical`, `info`, `success`, `neutral`, and `primary` to match the existing `alarm` and `warning` pattern.
🤖 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.
Duplicate comments:
In `@packages/core/src/components/blind/blind.scss`:
- Around line 18-26: The blind status border token map is inconsistent:
`blind.scss` mixes border-specific `*-bdr` tokens with plain color tokens in
`$blind-status-border`. Update the `$blind-status-border` entries in this map so
all status borders use the correct border-specific theme token naming, and
verify the exact token names against the theme package/design spec for
`critical`, `info`, `success`, `neutral`, and `primary` to match the existing
`alarm` and `warning` pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad890823-d269-41dc-a9ce-18432cf28792
⛔ Files ignored due to path filters (5)
testing/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-basic-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-basic-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-collapsed-1-chromium---classic-dark-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-collapsed-1-chromium---classic-light-linux.pngis excluded by!**/*.pngtesting/visual-testing/__screenshots__/tests/blind/blind.e2e.ts/blind-custom-header-1-chromium---classic-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (1)
packages/core/src/components/blind/blind.scss
Added fix for this and updated the snapshots |
nuke-ellington
left a comment
nuke-ellington
left a comment
There was a problem hiding this comment.
- Now border color vanished during collapse animation
- Please look at remaining open comments
For the collapse-animation comment: the disappearing border was addressed without keeping a real border on the collapsed layout box. Now it fades with opacity during collapse. |
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 (2)
packages/core/src/components/card-accordion/card-accordion.scss (2)
104-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead margins on the absolutely positioned footer.
margin-top: autoandmargin-bottom: 0pxhave no effect on an element withposition: absolute. These are leftover from the previous flexbox-based layout.♻️ Proposed fix
.expand-content-footer { display: block; position: absolute; bottom: 0; left: 0; width: 100%; height: 1rem; - - margin-top: auto; - margin-bottom: 0px; - background-color: var(--theme-color-component-1);🤖 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/card-accordion/card-accordion.scss` around lines 104 - 118, Remove the dead margin rules from the absolutely positioned footer in .expand-content-footer. Since this element uses position: absolute, drop the leftover margin-top and margin-bottom declarations and keep the positioning/layout styles that actually apply.
92-102: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the duplicate
.expand-content.show .expand-content-bodyrule.The first rule (lines 92–96) sets
display: noneandoverflow: hidden, both of which are immediately overridden by the second rule (lines 98–102) settingdisplay: blockandoverflow: auto. Onlycolorfrom the first rule survives. Merge them into a single rule.♻️ Proposed fix
.expand-content.show .expand-content-body { - display: none; - overflow: hidden; - color: var(--theme-color-std-text); - } - - .expand-content.show .expand-content-body { display: block; overflow: auto; height: calc(14.75rem - 1rem); + color: var(--theme-color-std-text); }🤖 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/card-accordion/card-accordion.scss` around lines 92 - 102, The card accordion stylesheet has two duplicate `.expand-content.show .expand-content-body` rules, and the first one is immediately overridden except for `color`. Consolidate the behavior in `card-accordion.scss` by merging the needed properties into a single `.expand-content.show .expand-content-body` block so the final styles are defined once and the duplicate rule is removed.
🤖 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/card-accordion/card-accordion.scss`:
- Line 82: The hardcoded expanded height value is duplicated in the card
accordion styles and should be replaced with a shared CSS custom property.
Define a custom property for the expanded height in card-accordion.scss and use
it in both the `.expand-content.show` rule and the `.expand-content-body` height
calculation so the value stays in sync and can be overridden easily.
---
Outside diff comments:
In `@packages/core/src/components/card-accordion/card-accordion.scss`:
- Around line 104-118: Remove the dead margin rules from the absolutely
positioned footer in .expand-content-footer. Since this element uses position:
absolute, drop the leftover margin-top and margin-bottom declarations and keep
the positioning/layout styles that actually apply.
- Around line 92-102: The card accordion stylesheet has two duplicate
`.expand-content.show .expand-content-body` rules, and the first one is
immediately overridden except for `color`. Consolidate the behavior in
`card-accordion.scss` by merging the needed properties into a single
`.expand-content.show .expand-content-body` block so the final styles are
defined once and the duplicate rule is removed.
🪄 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: cb764486-e130-4df9-8f36-31735fd217de
📒 Files selected for processing (2)
packages/core/src/components/card-accordion/card-accordion.scsspackages/core/src/components/push-card/push-card.scss
|
💡 What is the current behavior?
No borders present for alarm, success, other validation variants.
Outline variant behavior is different from figma design.
GitHub Issue Number: #
Jira Ticket- IX-4047
🆕 What is the new behavior?
Added CSS changes to reflect figma design better
🏁 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
Bug Fixes
UX Improvements