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(Dialog): improve accessibility #849

Merged
merged 5 commits into from
Nov 21, 2024
Merged

🚀 feat(Dialog): improve accessibility #849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f60ab79
feat(Dialog): improve accessibility
leonardo-gympass Nov 18, 2024
999d8d0
feat(Dialog): add closeLabel prop
leonardo-gympass Nov 18, 2024
fc2b201
fix(Dialog): add lock props to FocusLock
leonardo-gympass Nov 18, 2024
13e6081
test: update snapshots
leonardo-gympass Nov 18, 2024
0234aad
test(Dialog): add case for element focus
leonardo-gympass Nov 18, 2024
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
152 changes: 76 additions & 76 deletions packages/doc/content/components/components/dialog/index.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Gympass `<Dialog />` is a component which has the purpose of providing prioritar
- `<Dialog.Footer />`

### Recommendations of use Buttons

The right positioning of the buttons starting with the primary followed by the secondary

### Usage
Expand All @@ -37,18 +38,15 @@ render(() => {
<>
<Button onClick={handleOpen}>Dialog with one action</Button>

<Dialog
isOpen={isOpen}
>
<Dialog.Header>
We couldn't confirm your eligibility
</Dialog.Header>
<Dialog.Content>
Double check your info and company selection. If it still doesn’t work, contact your HR team.
</Dialog.Content>
<Dialog.Footer>
<Button onClick={handleOnClose}>Ok, I understand</Button>
</Dialog.Footer>
<Dialog isOpen={isOpen}>
<Dialog.Header>We couldn't confirm your eligibility</Dialog.Header>
<Dialog.Content>
Double check your info and company selection. If it still doesn’t
work, contact your HR team.
</Dialog.Content>
<Dialog.Footer>
<Button onClick={handleOnClose}>Ok, I understand</Button>
</Dialog.Footer>
</Dialog>
</>
);
Expand All @@ -71,21 +69,22 @@ render(() => {

return (
<>
<Button onClick={handleOpen} secondary>Dialog with two actions</Button>

<Dialog
isOpen={isOpen}
>
<Dialog.Header>
Are you sure you want to downgrade to a Platinum plan?
</Dialog.Header>
<Dialog.Content>
Double check your info and company selection. If it still doesn’t work, contact your HR team.
</Dialog.Content>
<Dialog.Footer>
<Button>Yes, downgrade</Button>
<Button.Text onClick={handleOnClose}>Cancel</Button.Text>
</Dialog.Footer>
<Button onClick={handleOpen} secondary>
Dialog with two actions
</Button>

<Dialog isOpen={isOpen}>
<Dialog.Header>
Are you sure you want to downgrade to a Platinum plan?
</Dialog.Header>
<Dialog.Content>
Double check your info and company selection. If it still doesn’t
work, contact your HR team.
</Dialog.Content>
<Dialog.Footer>
<Button>Yes, downgrade</Button>
<Button.Text onClick={handleOnClose}>Cancel</Button.Text>
</Dialog.Footer>
</Dialog>
</>
);
Expand All @@ -110,20 +109,18 @@ render(() => {
<>
<Button onClick={handleOpen}>Dialog with close button</Button>

<Dialog
isOpen={isOpen}
onClose={handleOnClose}
>
<Dialog.Header>
Are you sure you want to downgrade to a Platinum plan?
</Dialog.Header>
<Dialog.Content>
The change will happen at the end of your billing cycle on March, 28. Until then, enjoy your Black plan - you’ve already paid for it.
</Dialog.Content>
<Dialog.Footer>
<Dialog isOpen={isOpen} closeLabel="Close" onClose={handleOnClose}>
<Dialog.Header>
Are you sure you want to downgrade to a Platinum plan?
</Dialog.Header>
<Dialog.Content>
The change will happen at the end of your billing cycle on March, 28.
Until then, enjoy your Black plan - you’ve already paid for it.
</Dialog.Content>
<Dialog.Footer>
<Button>Yes, downgrade</Button>
<Button.Text onClick={handleOnClose}>Cancel</Button.Text>
</Dialog.Footer>
</Dialog.Footer>
</Dialog>
</>
);
Expand All @@ -146,18 +143,18 @@ render(() => {

return (
<>
<Button onClick={handleOpen} secondary>Dialog with only title</Button>

<Dialog
isOpen={isOpen}
>
<Dialog.Header>
Are you sure you want to downgrade to a Platinum plan?
</Dialog.Header>
<Dialog.Footer>
<Button onClick={handleOpen} secondary>
Dialog with only title
</Button>

<Dialog isOpen={isOpen}>
<Dialog.Header>
Are you sure you want to downgrade to a Platinum plan?
</Dialog.Header>
<Dialog.Footer>
<Button>Yes, downgrade</Button>
<Button.Text onClick={handleOnClose}>Cancel</Button.Text>
</Dialog.Footer>
</Dialog.Footer>
</Dialog>
</>
);
Expand All @@ -183,26 +180,25 @@ render(() => {
};

useEffect(() => {
if(isOpen && dialogRef.current && buttonRef.current) {
if (isOpen && dialogRef.current && buttonRef.current) {
buttonRef.current.focus();
}
},[dialogRef, buttonRef, isOpen]);
}, [dialogRef, buttonRef, isOpen]);

return (
<>
<Button onClick={handleOpen} secondary>Dialog with custom ref</Button>

<Dialog
isOpen={isOpen}
ref={dialogRef}
>
<Dialog.Header>
I am an example on how a custom ref option can be used
</Dialog.Header>
<Dialog.Footer>
<Button onClick={handleOpen} secondary>
Dialog with custom ref
</Button>

<Dialog isOpen={isOpen} ref={dialogRef}>
<Dialog.Header>
I am an example on how a custom ref option can be used
</Dialog.Header>
<Dialog.Footer>
<Button ref={buttonRef}>I am auto focused</Button>
<Button.Text onClick={handleOnClose}>Close</Button.Text>
</Dialog.Footer>
</Dialog.Footer>
</Dialog>
</>
);
Expand All @@ -228,27 +224,31 @@ render(() => {

return (
<>
<Button onClick={handleOpen} secondary>Dialog with custom id</Button>
<Button onClick={handleOpen} secondary>
Dialog with custom id
</Button>

<Dialog isOpen={isOpen}>
<Dialog.Header>
With a custom id I can open an isolated dialog
</Dialog.Header>

<Dialog.Footer>
<Button onClick={() => setIsNestedOpen(true)}>Open second dialog</Button>
<Dialog.Header>
With a custom id I can open an isolated dialog
</Dialog.Header>

<Dialog.Footer>
<Button onClick={() => setIsNestedOpen(true)}>
Open second dialog
</Button>
<Button.Text onClick={handleOnClose}>Close</Button.Text>
</Dialog.Footer>
</Dialog.Footer>
</Dialog>

<Dialog isOpen={isNestedOpen} dialogId="nested-id">
<Dialog.Header>
I am a nested dialog
</Dialog.Header>
<Dialog.Header>I am a nested dialog</Dialog.Header>

<Dialog.Footer>
<Button.Text onClick={() => setIsNestedOpen(false)}>Close</Button.Text>
</Dialog.Footer>
<Dialog.Footer>
<Button.Text onClick={() => setIsNestedOpen(false)}>
Close
</Button.Text>
</Dialog.Footer>
</Dialog>
</>
);
Expand Down
1 change: 1 addition & 0 deletions packages/yoga/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"prop-types": "^15.7.2",
"proxy-polyfill": "^0.3.1",
"rc-slider": "^8.7.1",
"react-focus-lock": "^2.13.2",
"react-google-font-loader": "^1.1.0",
"react-phone-input-2": "^2.15.1"
},
Expand Down
18 changes: 16 additions & 2 deletions packages/yoga/src/BottomSheet/web/__snapshots__/BottomSheet.test.jsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,14 +202,23 @@ exports[`<BottomSheet /> should match snapshot 1`] = `
<div
class="c0"
>
<div
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="0"
/>
<section
aria-modal="true"
class="c1 c2 c3"
data-focus-lock-disabled="false"
role="dialog"
>
<header
<h2
class="c4 c5"
tabindex="-1"
>
Title
</header>
</h2>
<p
class="c6 c7"
>
Expand All @@ -226,6 +235,11 @@ exports[`<BottomSheet /> should match snapshot 1`] = `
</button>
</footer>
</section>
<div
data-focus-guard="true"
style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;"
tabindex="0"
/>
</div>
</div>
</div>
Expand Down
36 changes: 33 additions & 3 deletions packages/yoga/src/Dialog/web/Dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import React, { useEffect, useCallback } from 'react';
import { createPortal } from 'react-dom';
import FocusLock from 'react-focus-lock';
import styled, { css } from 'styled-components';
import { func, bool, node, number, string } from 'prop-types';

import { Close } from '@gympass/yoga-icons';
import { usePortal, useCombinedRefs } from '../../hooks';
import { Button, Card, Box } from '../..';
import { focusOnFirstProgrammaticFocusableElement } from './utils';

export const StyledDialog = styled(Card)`
${({
Expand Down Expand Up @@ -78,12 +80,28 @@ const CloseButton = styled(Button.Icon)`

const Dialog = React.forwardRef(
(
{ isOpen, hideCloseButton, children, dialogId, onClose, zIndex, ...props },
{
isOpen,
hideCloseButton,
children,
dialogId,
onClose,
zIndex,
closeLabel,
className,
...props
},
forwardedRef,
) => {
const dialogRef = useCombinedRefs(forwardedRef);
const dialogElement = usePortal(dialogId ?? 'dialog');
const isCloseButtonVisible = onClose && !hideCloseButton;
const lockProps = {
role: 'dialog',
'aria-modal': true,
onClose,
...props,
};

const closeDialog = useCallback(
e => {
Expand Down Expand Up @@ -119,10 +137,18 @@ const Dialog = React.forwardRef(
ref={dialogRef}
zIndex={zIndex}
>
<StyledDialog onClose={onClose} {...props}>
<FocusLock
as={StyledDialog}
lockProps={lockProps}
returnFocus
disabled={!isOpen}
className={className}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the use of className here? Can this allow style customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose was to maintain compatibility with the current API of the component to avoid any side effects. Currently all props are passed to the Dialog, I kept this behavior in the lockProps, but the className is not accepted in this prop, which is why I had to create this exception.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand, I think we can keep this prop for now for safety and in the future we make it more restricted, no problem.

onActivation={focusOnFirstProgrammaticFocusableElement}
nataliosilveiraext marked this conversation as resolved.
Show resolved Hide resolved
>
{isCloseButtonVisible && (
<Box d="flex" justifyContent="flex-end" w="100%">
<CloseButton
aria-label={closeLabel}
icon={Close}
inverted
secondary
Expand All @@ -131,7 +157,7 @@ const Dialog = React.forwardRef(
</Box>
)}
{children}
</StyledDialog>
</FocusLock>
</Overlay>,
dialogElement,
)
Expand All @@ -150,6 +176,8 @@ Dialog.propTypes = {
hideCloseButton: bool,
/** Function to close the dialog. */
onClose: func,
closeLabel: string,
className: string,
zIndex: number,
children: node.isRequired,
};
Expand All @@ -158,6 +186,8 @@ Dialog.defaultProps = {
isOpen: false,
hideCloseButton: false,
onClose: undefined,
closeLabel: undefined,
className: undefined,
zIndex: 3,
dialogId: undefined,
};
Expand Down
16 changes: 14 additions & 2 deletions packages/yoga/src/Dialog/web/Dialog.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ describe('<Dialog />', () => {

render(
<ThemeProvider>
<Dialog isOpen onClose={onCloseMock}>
<Dialog isOpen onClose={onCloseMock} closeLabel="Close">
<Dialog.Header>Title</Dialog.Header>
</Dialog>
</ThemeProvider>,
);
const button = screen.getByRole('button');
const button = screen.getByRole('button', { name: 'Close' });

fireEvent.click(button);

Expand Down Expand Up @@ -111,4 +111,16 @@ describe('<Dialog />', () => {
expect(screen.getByText('Title')).toBeInTheDocument();
expect(screen.getByText('Second Title')).toBeInTheDocument();
});

it('should focus on the title when dialog is opened', () => {
render(
<ThemeProvider>
<Dialog isOpen>
<Dialog.Header>Add e-mail</Dialog.Header>
</Dialog>
</ThemeProvider>,
);

expect(document.activeElement).toHaveTextContent('Add e-mail');
});
});
2 changes: 1 addition & 1 deletion packages/yoga/src/Dialog/web/Header.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import Text from '../../Text';

const Header = props => (
<Text.H4 bold as="header" ta="center" mb="large" {...props} />
<Text.H4 bold as="h2" ta="center" mb="large" tabIndex={-1} {...props} />
nataliosilveiraext marked this conversation as resolved.
Show resolved Hide resolved
);

Header.displayName = 'Dialog.Header';
Expand Down
Loading
Loading