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.

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

fix(LEMS-2225): add conditional aria-describedby logic #2283

Merged
merged 12 commits into from
Aug 1, 2024
5 changes: 5 additions & 0 deletions .changeset/plenty-rockets-breathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-popover": patch
---

Testing adding aria-describedby
anakaren-rojas marked this conversation as resolved.
Show resolved Hide resolved
52 changes: 50 additions & 2 deletions __docs__/wonder-blocks-popover/popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Button from "@khanacademy/wonder-blocks-button";
import {View} from "@khanacademy/wonder-blocks-core";
import {Strut} from "@khanacademy/wonder-blocks-layout";
import {color, spacing} from "@khanacademy/wonder-blocks-tokens";
import {LabelLarge} from "@khanacademy/wonder-blocks-typography";
import {HeadingMedium, LabelLarge} from "@khanacademy/wonder-blocks-typography";
import type {Placement} from "@khanacademy/wonder-blocks-tooltip";

import {Popover, PopoverContent} from "@khanacademy/wonder-blocks-popover";
Expand Down Expand Up @@ -87,6 +87,16 @@ const styles = StyleSheet.create({
flexDirection: "row",
gap: spacing.medium_16,
},
srOnly: {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Does WonderBlocks have a utility constant for this? It would be great if it was in a shared file for other WonderBlocks components to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem like it; I'd imagine since wonderblocks hosts components, they would all need to be visible

border: 0,
clip: "rect(0,0,0,0)",
height: 1,
margin: -1,
overflow: "hidden",
padding: 0,
position: "absolute",
width: 1,
},
});

type StoryComponentType = StoryObj<typeof Popover>;
Expand Down Expand Up @@ -729,7 +739,7 @@ export const PopoverAlignment: StoryComponentType = {
* With custom aria-label - overrides the default aria-describedby and aria-labelledby
*/

export const CustomAriaLabel: StoryComponentType = {
export const WithCustomAriaLabel: StoryComponentType = {
args: {
children: <Button>Open popover</Button>,
content: ContentMappings.withTextOnly,
Expand All @@ -742,3 +752,41 @@ export const CustomAriaLabel: StoryComponentType = {
"aria-label": "Popover with custom aria label",
} as PopoverArgs,
};

export const WithAriaDescribedBy = ({placement}: {placement: Placement}) => {
const [opened, setOpened] = React.useState(false);

return (
<View style={styles.example}>
<Popover
id="Popover"
placement={placement}
opened={opened}
onClose={() => setOpened(false)}
content={
<>
<HeadingMedium
id="Popover-content"
style={styles.srOnly}
>
Hidden text that would describe the popover content
</HeadingMedium>
<PopoverContent
title="Title"
content="Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip commodo."
closeButtonVisible
/>
</>
}
>
<Button
onClick={() => {
setOpened(true);
}}
>
{`Open popover`}
</Button>
</Popover>
</View>
);
};
14 changes: 12 additions & 2 deletions packages/wonder-blocks-popover/src/components/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,20 @@ export default class Popover extends React.Component<Props, State> {
showTail,
portal,
"aria-label": ariaLabel,
"aria-describedby": ariaDescribedBy,
} = this.props;
const {anchorElement} = this.state;

const ariaDescribedBy = ariaLabel ? undefined : `${uniqueId}-content`;
/**
* If aria-describedby is provided, use that id as a reference to the content
* If only aria-label is provided, don't use aria-describedby
* If both are provided, use aria-label as the label (automatically done by SR)
* If neither are provided, use the uniqueId to reference the content
* > Don't want to have aria-describedby set by default in case there isn't something
* > for the SR to reference.
*/
anakaren-rojas marked this conversation as resolved.
Show resolved Hide resolved
const describedBy =
ariaDescribedBy || (ariaLabel ? undefined : `${uniqueId}-content`);
const ariaLabelledBy = ariaLabel ? undefined : `${uniqueId}-title`;

const popperContent = (
Expand All @@ -290,7 +300,7 @@ export default class Popover extends React.Component<Props, State> {
<PopoverDialog
{...props}
aria-label={ariaLabel}
aria-describedby={ariaDescribedBy}
aria-describedby={describedBy}
aria-labelledby={ariaLabelledBy}
id={uniqueId}
onUpdate={(placement) => this.setState({placement})}
Expand Down
Loading