feat(Card): improve the accessibility of the component #4585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-sarka-card-a11y-fix.surge.sh |
|
Size Change: +79 B (+0.02%) Total Size: 459 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
2abcca2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://341673e6.orbit.pages.dev |
| Branch Preview URL: | https://sarka-card-a11y-fix.orbit.pages.dev |
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
2 times, most recently
from
d847976 to
55b517f
Compare
|
Storybook staging is available at https://kiwicom-orbit-sarka-card-a11y-fix.surge.sh Playroom staging is available at https://kiwicom-orbit-sarka-card-a11y-fix.surge.sh/playroom |
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
4 times, most recently
from
9db2842 to
1afd500
Compare
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
from
1afd500 to
c0a5a49
Compare
sarkaaa
commented
Jan 24, 2025
| <Stack inline grow={false} justify="end"> | ||
| {actions} | ||
| </Stack> | ||
| const InteractiveWrapper = ({ |
There was a problem hiding this comment.
I've created this component to avoid code duplication (rendered 3 times within this file).
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
from
c0a5a49 to
879c2b8
Compare
sarkaaa
commented
Jan 24, 2025
| /> | ||
| {actions && <Actions actions={actions} />} | ||
| <div className="p-400 lm:p-600 "> | ||
| <InteractiveWrapper onClick={!children ? onClick : undefined}> |
There was a problem hiding this comment.
I added this !children condition, bcs in case that the CardSection has both - header and children and onClick, then the CardSection component is focusable twice.
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
from
879c2b8 to
79ce698
Compare
3 tasks
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
3 times, most recently
from
2c8b08f to
850a2d5
Compare
sarkaaa
changed the title
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
from
850a2d5 to
2abcca2
Compare
sarkaaa
commented
Jan 28, 2025
| @@ -339,12 +340,19 @@ export const CardWithMixedSections: Story = { | |||
| <CardSection title={sectionTitle} description={sectionDescription}> | |||
| Section Content | |||
| </CardSection> | |||
| <CardSection onClick={onClick} title={sectionTitle} description={sectionDescription}> | |||
There was a problem hiding this comment.
I added a variant with CardSection with onClick function, I haven't found this variant in SB.
There was a problem hiding this comment.
I would also add stories for onClick together with actions, both non/expanded.
There was a problem hiding this comment.
I added these to the story so I can check all cases, so all my comments in the review are based on also these
<Card>
<CardSection
actions={<ButtonLink onClick={handleClick}>Open</ButtonLink>}
onClick={onClick}
expandable={false}
title={sectionTitle}
>
Section Content with onClick
</CardSection>
<CardSection
actions={<ButtonLink onClick={handleClick}>Open</ButtonLink>}
onClick={onClick}
expandable
title={sectionTitle}
>
Section Content with onClick
</CardSection>
</Card>
| @@ -339,12 +340,19 @@ export const CardWithMixedSections: Story = { | |||
| <CardSection title={sectionTitle} description={sectionDescription}> | |||
| Section Content | |||
| </CardSection> | |||
| <CardSection onClick={onClick} title={sectionTitle} description={sectionDescription}> | |||
There was a problem hiding this comment.
I would also add stories for onClick together with actions, both non/expanded.
| import Stack from "../../Stack"; | ||
| import handleKeyDown from "../../utils/handleKeyDown"; | ||
|
|
||
| const ContentWrapper = ({ |
There was a problem hiding this comment.
If I understand correctly, you created this Wrapper to solve nested interactive elements when both actions and onClick are provided?
| onKeyDown={onClick ? handleKeyDown(onClick) : undefined} | ||
| onClick={onClick || undefined} | ||
| className={cx( | ||
| "orbit-card-wrapper-button flex-1 focus:outline-none", |
There was a problem hiding this comment.
I find it odd that there is class "orbit-card-wrapper-button" even if this is not a button & in case of title/header and children, it's present there twice 🤔
| <div | ||
| className={cx( | ||
| "duration-fast lm:border-x border-b transition-all ease-in-out", | ||
| "duration-fast lm:border-x relative border-b transition-all ease-in-out", |
There was a problem hiding this comment.
When the card is not expandable, the focus/outline animation is rather weird and slow compared to expandable card, probably caused by the transition duration? It also looks like there is black border/outline added right before the blue one. And when section is expandable, the grey border disappears and is replaced by the blue one, not happening when not expandable. Can you take a look at these?
| onClick != null && "hover:bg-white-normal-hover cursor-pointer", | ||
| onClick && !expandable && "hover:bg-white-normal-hover", | ||
| onClick && | ||
| "has-[.orbit-card-wrapper-button:focus]:focus-within:rounded-100 has-[.orbit-card-wrapper-button:focus]:focus-within:outline-blue-normal has-[.orbit-card-wrapper-button:focus]:focus-within:outline has-[.orbit-card-wrapper-button:focus]:focus-within:outline-2", |
There was a problem hiding this comment.
Bottom outline/border is broken when the section is expanded 😬 Can you check, please?
| {actions && <Actions actions={actions} />} | ||
| </div> | ||
| )} | ||
|
|
||
| {children && expandable && ( | ||
| <Expandable expanded={opened} slideID={slideID} labelID={slideID}> | ||
| <div className="font-base text-normal text-primary-foreground px-400 lm:px-600 w-full leading-normal"> | ||
| <div className="py-400 lm:py-600 border-elevation-flat-border-color border-t"> | ||
| <ContentWrapper | ||
| onClick={opened ? onClick : undefined} |
There was a problem hiding this comment.
Is this change on purpose? Because currently on production, when section is opened, onClick event is not fired.
Shouldn't be there also grey hover effect on this section when expanded, same as on production? Currently it's only on the Header part.
| // See comment above | ||
| // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex | ||
| tabIndex={onClick == null ? undefined : 0} | ||
| onClick={onClick} |
There was a problem hiding this comment.
As you removed this, now when card is expandable, onClick is not fired when clicked on the header, is that desired change?
| expanded={opened} | ||
| isSection | ||
| /> | ||
| </ContentWrapper> | ||
| {actions && <Actions actions={actions} />} |
There was a problem hiding this comment.
In this case, when actions are set both with onClick, it's not possible to click on the clickable element in the actions 😬
| @@ -105,42 +121,45 @@ export default function CardSection({ | |||
| {actions && <Actions actions={actions} />} | |||
| </div> | |||
| )} | |||
|
|
|||
| {(title != null || header != null) && !expandable && ( | |||
There was a problem hiding this comment.
I am a bit unsure about the focus order in this case (when both onClick and actions are provided), first the action button is being focused and then the card section is being focused, shouldn't it be the other way around? Same as when it's expandable? 🤔
| onClick != null && "hover:bg-white-normal-hover cursor-pointer", | ||
| onClick && !expandable && "hover:bg-white-normal-hover", | ||
| onClick && | ||
| "has-[.orbit-card-wrapper-button:focus]:focus-within:rounded-100 has-[.orbit-card-wrapper-button:focus]:focus-within:outline-blue-normal has-[.orbit-card-wrapper-button:focus]:focus-within:outline has-[.orbit-card-wrapper-button:focus]:focus-within:outline-2", |
There was a problem hiding this comment.
Perhaps you could also add here has-[.orbit-card-wrapper-button:focus]:focus-within:z-10? Border of other card sections is on top of the outline 😬
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes https://kiwicom.atlassian.net/browse/FEPLT-2214
This PR enhances the accessibility of the
CardSectioncomponent which was done before - #4574 . The PR avoids the duplication ofactionsprop and setsonClickfunction only tochildrenelement.As agreed before, this PR doesn't cover the variant when
onClick !== null&& children contains an interactive element (button, link), that should be discussed as a scope of the component's functionality - check documentation.This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.ts✨
Description by Callstackai
This PR improves the accessibility of the
Cardcomponent by adding click handlers and enhancing keyboard navigation support.Diagrams of code changes
sequenceDiagram actor User participant CardSection participant ContentWrapper participant HandleKeyDown User->>CardSection: Interacts with section alt Has onClick prop CardSection->>ContentWrapper: Renders with onClick User->>ContentWrapper: Clicks or presses key alt Key pressed ContentWrapper->>HandleKeyDown: Process keydown event HandleKeyDown-->>ContentWrapper: Trigger onClick if Enter/Space else Mouse click ContentWrapper->>ContentWrapper: Direct onClick trigger end ContentWrapper-->>User: Visual feedback (hover/focus states) else No onClick prop CardSection->>ContentWrapper: Renders without interaction endFiles Changed
onClickprop to theCardcomponent and updated the story to include it.ContentWrappercomponent to handle click events and improve accessibility features.