fix(card-list): add keyboard navigation for "show more cards" card#2613
fix(card-list): add keyboard navigation for "show more cards" card#2613lakshmi-priya-b wants to merge 5 commits into
Conversation
|
✅ 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:
📝 WalkthroughWalkthroughThe CardList show-more keyboard accessibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 |
![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 improves the accessibility of the 'Show More' card in the CardList component by replacing the wrapper element with a native , adding appropriate ARIA attributes, and introducing focus-visible styles. It also adds automated accessibility and keyboard interaction tests. The review feedback recommends adding type="button" to the button to prevent unintended form submissions, replacing all: unset with explicit CSS resets to avoid layout inconsistencies, and adding a changeset as required by the repository style guide for accessibility updates.
[!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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/card-list/card-list.scss (1)
95-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix lint-blocking SCSS declarations in
.Show__All__Card.
appearance: buttonis flagged as deprecated by your Stylelint rule, and line formatting atcoloralso violates lint config. This can block CI.Suggested patch
.Show__All__Card { all: unset; box-sizing: border-box; - appearance: button; + appearance: auto; display: flex; @@ .Show__All__Card ix-card { width: 100%; height: 100%; --ix-card-border-color: var(--theme-color-primary); + color: var(--theme-color-primary); }🤖 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-list/card-list.scss` around lines 95 - 126, Remove the deprecated `appearance: button;` declaration from the `.Show__All__Card` selector styling block as it is flagged by your Stylelint rule. Additionally, fix the line formatting violation in the `.Show__All__Card ix-card` selector where the `color` property is declared - ensure it follows your project's lint configuration for proper indentation and spacing to prevent CI blocking.Source: Linters/SAST tools
🤖 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-list/test/card-list.ct.ts`:
- Around line 21-54: A required `renders` test is missing from this test file.
Add a new regressionTest called 'renders' between the 'accessibility' test and
the 'show more button can be activated via keyboard' test. This renders test
should mount the ix-card-list component with the CARDS_HTML content and verify
that the component hydrates correctly by checking that the ix-card-list element
has the hydrated class, similar to the hydration check already present in the
keyboard activation test.
- Around line 41-45: The keyboard test in card-list.ct.ts uses a regex pattern
without word boundaries and a class-based selector that should be replaced with
an accessible locator. Update the expect statement with toHaveClass to use the
word boundary regex pattern /\bhydrated\b/ instead of /hydrated/ to ensure
proper matching. Additionally, replace the cardList.locator with class selector
'.Show__All__Card' with cardList.getByRole('button', ...) to use
accessibility-based locators for the show-more button, which aligns with testing
best practices.
---
Outside diff comments:
In `@packages/core/src/components/card-list/card-list.scss`:
- Around line 95-126: Remove the deprecated `appearance: button;` declaration
from the `.Show__All__Card` selector styling block as it is flagged by your
Stylelint rule. Additionally, fix the line formatting violation in the
`.Show__All__Card ix-card` selector where the `color` property is declared -
ensure it follows your project's lint configuration for proper indentation and
spacing to prevent CI blocking.
🪄 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: 9c7cc2a3-e688-4261-b5b0-81451dc94468
📒 Files selected for processing (3)
packages/core/src/components/card-list/card-list.scsspackages/core/src/components/card-list/card-list.tsxpackages/core/src/components/card-list/test/card-list.ct.ts
lakshmi-priya-b
force-pushed
the
fix/card-list-show-more-card-accessibility
branch
from
5795fec to
b623471
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/card.scss`:
- Around line 126-132: In the :host(:focus-visible) rule in card.scss, remove
the empty line between the outline declaration and outline-offset to satisfy the
declaration-empty-line-before Stylelint rule; keep the declarations for outline,
outline-offset, and border-radius grouped together in that block.
In `@packages/core/src/components/card/test/card.ct.ts`:
- Around line 13-49: The card regression tests cover accessibility and
attributes but do not verify keyboard activation behavior for non-passive cards.
Add a direct interaction regression test in card.ct.ts using the existing
regressionTest and ix-card locator to mount a non-passive ix-card, focus it, and
assert that Enter and Space trigger the expected activation/click behavior,
while keeping the passive case unaffected. Use the ix-card and ix-card-content
setup already present so the new test validates the updated keyboard handling
end to end.
🪄 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: 0f8c785c-4084-4582-87ab-63afdf323b4c
📒 Files selected for processing (3)
packages/core/src/components/card/card.scsspackages/core/src/components/card/card.tsxpackages/core/src/components/card/test/card.ct.ts
lakshmi-priya-b
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/card.ct.ts`:
- Around line 61-79: The click counter assertion in the card test still uses
page.evaluate; replace the final manual readback with a locator-based assertion
on card so the test checks __clickCount directly via the locator API, such as
using expect(card).toHaveJSProperty('__clickCount', 2). Keep the existing setup
in the card.ct test that attaches the click listener, and only change the
verification step to use the locator assertion.
🪄 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: 3f418fea-c991-42f8-b87c-7e5aa8acbdf4
📒 Files selected for processing (2)
packages/core/src/components/card/card.scsspackages/core/src/components/card/test/card.ct.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/card/card.scss
| await page.evaluate(() => { | ||
| const el = document.querySelector('ix-card') as HTMLIxCardElement & { | ||
| __clickCount: number; | ||
| }; | ||
| el.__clickCount = 0; | ||
| el.addEventListener('click', () => { | ||
| el.__clickCount += 1; | ||
| }); | ||
| }); | ||
|
|
||
| await card.press('Enter'); | ||
| await card.press(' '); | ||
|
|
||
| const clickCount = await page.evaluate(() => { | ||
| const el = document.querySelector('ix-card') as HTMLIxCardElement & { | ||
| __clickCount: number; | ||
| }; | ||
| return el.__clickCount; | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline packages/core/src/components/card/test/card.ct.ts --view expanded || true
echo
echo "== relevant lines =="
sed -n '1,180p' packages/core/src/components/card/test/card.ct.ts
echo
echo "== search for similar click-count patterns in component tests =="
rg -n "toHaveAttribute\\(|page\\.evaluate\\(|__clickCount|addEventListener\\('click'|regressionTest\\(" packages/core/src/components -g '*.ct.ts' | sed -n '1,220p'Repository: siemens/ix
Length of output: 25668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file metadata =="
wc -l packages/core/src/components/card/test/card.ct.ts
echo
echo "== SPDX header check =="
head -n 5 packages/core/src/components/card/test/card.ct.ts
echo
echo "== related test helper usage =="
rg -n "makeAxeBuilder|regressionTest|test\\(" packages/core/src/components/card/test/card.ct.ts packages/core/src/components -g '*.ct.ts' | sed -n '1,200p'Repository: siemens/ix
Length of output: 20797
Use a locator assertion for the click counter packages/core/src/components/card/test/card.ct.ts:61-79 still reads the count via page.evaluate; switch the final assertion to a locator-based check, e.g. await expect(card).toHaveJSProperty('__clickCount', 2).
🤖 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/test/card.ct.ts` around lines 61 - 79, The
click counter assertion in the card test still uses page.evaluate; replace the
final manual readback with a locator-based assertion on card so the test checks
__clickCount directly via the locator API, such as using
expect(card).toHaveJSProperty('__clickCount', 2). Keep the existing setup in the
card.ct test that attaches the click listener, and only change the verification
step to use the locator assertion.
Source: Path instructions
lakshmi-priya-b
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/components/card-list/card-list.scss (1)
122-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge this back into the existing
.Show__All__Cardblock.The second selector only adds pseudo-state styles, but it keeps Sonar reporting a duplicate selector and splits the card styles across two blocks.
🤖 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-list/card-list.scss` around lines 122 - 134, The `.Show__All__Card` styles are split across duplicate selector blocks, which triggers Sonar and makes the card styling harder to follow. Merge the pseudo-state rules from the second `.Show__All__Card` block back into the existing `.Show__All__Card` definition in card-list.scss, keeping the `&:hover`, `&:active`, and `@include hover.focus-visible` rules together under the same selector.Source: Linters/SAST tools
packages/core/src/components/card-list/test/card-list.ct.ts (1)
204-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that hidden cards become visible, not just that the overflow card disappears.
not.toBeVisible()only proves the overflow state flipped. It won't catch regressions where the slotted cards stay hidden. Add a visibility check for one of the previously hidden cards after both Enter and Space. As per coding guidelines,packages/core/src/components/**/test/*.ct.ts: “Use core component tests for component behavior, interaction, keyboard handling, accessibility, slots, events, and state changes.”Suggested patch
await showMoreCard.focus(); await page.keyboard.press('Enter'); await expect(showMoreCard).not.toBeVisible(); + await expect(cardList.getByText('Card 4')).toBeVisible(); + await expect(cardList.getByText('Card 5')).toBeVisible(); const showLessButton = cardList.getByRole('button', { name: /show less/i }); await showLessButton.click(); showMoreCard = cardList.locator('.Show__All__Card'); await expect(showMoreCard).toBeVisible(); await showMoreCard.focus(); await page.keyboard.press(' '); await expect(showMoreCard).not.toBeVisible(); + await expect(cardList.getByText('Card 4')).toBeVisible(); + await expect(cardList.getByText('Card 5')).toBeVisible();🤖 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-list/test/card-list.ct.ts` around lines 204 - 214, In the card-list keyboard interaction test, the current assertions only verify that the overflow “show more” card hides, but they do not confirm that previously hidden slotted cards become visible again. Update the test around the Show__All__Card interactions in card-list.ct.ts to also assert visibility of one of the hidden cards after pressing Enter and again after pressing Space. Use the existing cardList, showMoreCard, and showLessButton flow to locate the affected behavior and make sure the visibility checks cover the actual card content, not just the overflow control state.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/card-list/card-list.tsx`:
- Around line 200-207: The onShowMoreCardClick flow is casting keyboard
activations to MouseEvent, which makes the showMoreCardClick payload expose the
wrong nativeEvent type. Update the event contract used by
CardList/showMoreCardClick to accept MouseEvent | KeyboardEvent (or UIEvent) and
propagate the real event through handleClick instead of casting in
onShowMoreCardClick, so consumers see the correct runtime object.
---
Nitpick comments:
In `@packages/core/src/components/card-list/card-list.scss`:
- Around line 122-134: The `.Show__All__Card` styles are split across duplicate
selector blocks, which triggers Sonar and makes the card styling harder to
follow. Merge the pseudo-state rules from the second `.Show__All__Card` block
back into the existing `.Show__All__Card` definition in card-list.scss, keeping
the `&:hover`, `&:active`, and `@include hover.focus-visible` rules together
under the same selector.
In `@packages/core/src/components/card-list/test/card-list.ct.ts`:
- Around line 204-214: In the card-list keyboard interaction test, the current
assertions only verify that the overflow “show more” card hides, but they do not
confirm that previously hidden slotted cards become visible again. Update the
test around the Show__All__Card interactions in card-list.ct.ts to also assert
visibility of one of the hidden cards after pressing Enter and again after
pressing Space. Use the existing cardList, showMoreCard, and showLessButton flow
to locate the affected behavior and make sure the visibility checks cover the
actual card content, not just the overflow control state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c6eeadd-c65c-4753-8517-acc23178d363
📒 Files selected for processing (3)
packages/core/src/components/card-list/card-list.scsspackages/core/src/components/card-list/card-list.tsxpackages/core/src/components/card-list/test/card-list.ct.ts
|
1 participant
💡 What is the current behavior?
"There are more cards available" overflow card in ix-card-list is not accessible through keyboard.
JIRA TICKET : IX-4349
🆕 What is the new behavior?
Made "There are more cards available" overflow card in ix-card-list accessibile through keyboard
🏁 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
New Features
Bug Fixes