Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Card): improve the accessibility of the component #4585

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(Card): improve the accessibility of the component #4585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/orbit-components/src/Card/Card.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type CardPropsAndCustomArgs = React.ComponentProps<typeof Card> & {
sectionTitle: string;
sectionDescription: string;
initialExpanded?: boolean;
onClick: () => void;
};

const meta: Meta<CardPropsAndCustomArgs> = {
Expand Down Expand Up @@ -312,7 +313,7 @@ export const CardWithDefaultExpanded: Story = {
};

export const CardWithMixedSections: Story = {
render: ({ sectionTitle, sectionDescription, ...args }) => (
render: ({ sectionTitle, sectionDescription, onClick, ...args }) => (
<Card
{...args}
actions={
Expand All @@ -339,12 +340,19 @@ export const CardWithMixedSections: Story = {
<CardSection title={sectionTitle} description={sectionDescription}>
Section Content
</CardSection>
<CardSection onClick={onClick} title={sectionTitle} description={sectionDescription}>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a variant with CardSection with onClick function, I haven't found this variant in SB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also add stories for onClick together with actions, both non/expanded.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Section Content with onClick
</CardSection>
</Card>
),

args: {
onClick: action("onClick"),
},

parameters: {
controls: {
exclude: ["labelClose", "initialExpanded", "expanded"],
exclude: ["labelClose", "initialExpanded", "expanded", "onClick"],
},
},
};
Expand Down
75 changes: 47 additions & 28 deletions packages/orbit-components/src/Card/CardSection/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,30 @@ import { ELEMENT_OPTIONS } from "../../Heading/consts";
import type { Props } from "./types";
import Header from "../components/Header";
import Expandable from "./components/Expandable";
import handleKeyDown from "../../utils/handleKeyDown";
import Stack from "../../Stack";
import handleKeyDown from "../../utils/handleKeyDown";

const ContentWrapper = ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, you created this Wrapper to solve nested interactive elements when both actions and onClick are provided?

onClick,
className,
children,
}: Pick<Props, "onClick" | "children"> & { className?: string }) => (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
role={onClick ? "button" : undefined}
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
tabIndex={onClick ? 0 : undefined}
onKeyDown={onClick ? handleKeyDown(onClick) : undefined}
onClick={onClick || undefined}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? Why not just onClick={onClick}?

className={cx(
"orbit-card-wrapper-button flex-1 focus:outline-none",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

onClick && "before:rounded-100 before:absolute before:inset-0",
className,
)}
>
{children}
</div>
);

const Actions = ({ actions }) => (
<Stack inline grow={false} justify="end">
Expand Down Expand Up @@ -58,26 +80,20 @@ export default function CardSection({

return (
// Needs to capture bubbled click events from the <button> below
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

opened && "my-200 rounded-100 shadow-level2 [&+*]:border-t",
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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bottom outline/border is broken when the section is expanded 😬 Can you check, please?

Snímek obrazovky 2025-01-30 v 14 28 20

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

Snímek obrazovky 2025-01-30 v 15 28 51

)}
data-test={dataTest}
role={onClick == null ? undefined : "button"}
// See comment above
// eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
tabIndex={onClick == null ? undefined : 0}
onClick={onClick}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you removed this, now when card is expandable, onClick is not fired when clicked on the header, is that desired change?

// Not needed once we can use <button> or <a> like we should
onKeyDown={onClick == null ? undefined : handleKeyDown(onClick)}
>
{(title != null || header != null) && expandable && (
<div
className={cx(
"hover:bg-white-normal-hover p-400 lm:p-600 gap-300 relative z-10 flex cursor-pointer items-center",
"hover:bg-white-normal-hover p-400 lm:p-600 gap-300 flex cursor-pointer items-center",
"has-[.orbit-card-header-button:focus]:focus-within:rounded-100 has-[.orbit-card-header-button:focus]:focus-within:outline-blue-normal has-[.orbit-card-header-button:focus]:focus-within:outline has-[.orbit-card-header-button:focus]:focus-within:outline-2",
)}
>
Expand Down Expand Up @@ -105,42 +121,45 @@ export default function CardSection({
{actions && <Actions actions={actions} />}
</div>
)}

{(title != null || header != null) && !expandable && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

<div className="p-400 lm:p-600 w-full">
<Header
title={title}
titleAs={titleAs}
description={description}
expandable={expandable}
header={header}
expanded={opened}
actions={actions}
isSection
/>
<div className="gap-300 p-400 lm:p-600 flex items-center ">
<ContentWrapper onClick={!children ? onClick : undefined}>
<Header
title={title}
titleAs={titleAs}
description={description}
header={header}
expanded={opened}
isSection
/>
</ContentWrapper>
{actions && <Actions actions={actions} />}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, when actions are set both with onClick, it's not possible to click on the clickable element in the 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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

className={cx("py-400 lm:py-600 border-elevation-flat-border-color border-t")}
>
{children}
</div>
</ContentWrapper>
</div>
</Expandable>
)}

{children && !expandable && (
<div
<ContentWrapper
className={cx(
"font-base text-normal text-primary-foreground px-400 lm:px-600 pb-400 lm:pb-600 w-full leading-normal",
title == null && header == null && "pt-400 lm:pt-600",
)}
onClick={onClick}
>
{children}
</div>
</ContentWrapper>
)}
</div>
);
Expand Down
Loading