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: +307 B (+0.07%) Total Size: 460 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
7a934f5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://69ae352e.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?
There was a problem hiding this comment.
yes, I created this ContentWrapper to avoid duplication, otherwise there would be same div with the same attributes (role, tabIndex, onClick, className ,... etc)..
| 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?
There was a problem hiding this comment.
Master:
master.mp4
Your branch:
staging.mp4
| 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 😬
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
2 times, most recently
from
d94278c to
8623a66
Compare
sarkaaa
force-pushed
the
sarka/card-a11y-fix
branch
from
8623a66 to
7a934f5
Compare
sarkaaa
commented
Feb 5, 2025
| @@ -307,7 +309,7 @@ export const CardWithMixedSections: Story = { | |||
| expandable | |||
| title={sectionTitle} | |||
| actions={ | |||
| <ButtonLink compact size="small" type="secondary"> | |||
| <ButtonLink onClick={buttonClick} compact size="small" type="secondary"> | |||
There was a problem hiding this comment.
I added this action('buttonClick') to be able to see that click action is working in SB.
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.UPDATE:
actionelements does not trigger the card's onClick.:beforehack as it's impossible to make a different implementation for this design of the CardSection component.Check Playroom with variants here.
✨
Description by Callstackai
This PR enhances the accessibility of the
CardSectioncomponent by improving click handling and avoiding duplication of theactionsprop.Diagrams of code changes
sequenceDiagram actor User participant Card participant CardSection participant ButtonLink participant ContentWrapper User->>Card: Clicks Card Section alt Section is Expandable CardSection->>ContentWrapper: Render with onClick handler ContentWrapper-->>User: Show clickable content User->>ContentWrapper: Clicks content ContentWrapper->>CardSection: Triggers onClick CardSection->>Card: Propagates onClick event end alt Section has Actions Card->>ButtonLink: Render action buttons User->>ButtonLink: Clicks action button ButtonLink->>Card: Triggers buttonClick event end alt Section is Non-expandable CardSection->>ContentWrapper: Render with hover state User->>ContentWrapper: Clicks content ContentWrapper->>CardSection: Triggers onClick CardSection->>Card: Propagates onClick event endFiles Changed
onClickandbuttonClickprops for better interaction handling.ContentWrappercomponent for handling click events and improved accessibility features.onClickprop.This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.md. See list of supported languages.