Skip to content

LG-4952: Updates Modal component to use dialog element #2579

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

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
01353ba
WIP
bruugey Dec 5, 2024
84b68db
Modal migration
bruugey Dec 5, 2024
6bed50e
with changesets and removing deps
bruugey Dec 5, 2024
49b4479
lints
bruugey Dec 5, 2024
46bfa16
with docs
bruugey Dec 9, 2024
ef96bd0
fix docs
bruugey Dec 9, 2024
dea1576
Merge branch 'main' into brooke/modal-updates
bruugey Dec 9, 2024
554012a
Merge branch 'main' into brooke/modal-updates
bruugey Dec 9, 2024
fbbcd03
runs fix
bruugey Dec 9, 2024
3a456a5
Merge remote-tracking branch 'origin' into brooke/modal-updates
bruugey Dec 9, 2024
ea44dad
Merge branch 'brooke/modal-updates' of github.com:mongodb/leafygreen-…
bruugey Dec 9, 2024
ebdccd7
Merge branch 'main' into brooke/modal-updates
bruugey Dec 12, 2024
d745726
WIP
bruugey Dec 12, 2024
60500f6
Merge remote-tracking branch 'origin' into brooke/modal-updates
bruugey Mar 4, 2025
bb78e5a
bring to latest
bruugey Mar 4, 2025
34b03b1
update pnpm lock
bruugey Mar 4, 2025
2c5eb46
update validate/build
bruugey Mar 4, 2025
1f45e50
Delete yarn.lock
bruugey Mar 4, 2025
da9e572
update id
bruugey Mar 4, 2025
cb6aaa3
Merge branch 'brooke/modal-updates' of github.com:mongodb/leafygreen-…
bruugey Mar 4, 2025
044e9df
Merge remote-tracking branch 'origin' into brooke/modal-updates
bruugey Mar 17, 2025
078576c
update
bruugey Mar 17, 2025
bab4129
Update packages/modal/README.md
bruugey Mar 17, 2025
a82531a
responding to PR feedback
bruugey Mar 17, 2025
d0bee20
responding to more PR feedback
bruugey Mar 18, 2025
c5aad57
with upgrade.md
bruugey Mar 18, 2025
59c46e2
Update packages/modal/src/Modal/Modal.tsx
bruugey Mar 18, 2025
0ff3c17
Update packages/modal/src/Modal/Modal.styles.ts
bruugey Mar 18, 2025
3306e4d
rm unnecc breakpoints
bruugey Mar 18, 2025
4427ddc
runs fix
bruugey Mar 18, 2025
85b8632
Merge remote-tracking branch 'origin' into brooke/modal-updates
bruugey Mar 20, 2025
6e9fab7
Merge remote-tracking branch 'origin' into brooke/modal-updates
bruugey Mar 25, 2025
7f23dfa
should fix confirmation modal styles
bruugey Mar 25, 2025
cce69e7
should fix marketing moadl too
bruugey Mar 25, 2025
f166348
merged
bruugey Apr 7, 2025
b39f9d6
fix chromatic
bruugey Apr 7, 2025
d70631f
Merge branch 'main' of github.com:mongodb/leafygreen-ui into brooke/m…
bruugey Apr 17, 2025
20c96e9
responding to PR feedback part one
bruugey Apr 17, 2025
2901037
rm variant decl
bruugey Apr 17, 2025
e59a668
attempt to fix visible bug
bruugey Apr 17, 2025
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
7 changes: 7 additions & 0 deletions .changeset/soft-files-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@leafygreen-ui/confirmation-modal': major
'@leafygreen-ui/marketing-modal': major
'@leafygreen-ui/modal': major
---

Upgrades components to use the native `dialog` HTML element. This means we no longer have to handle focus trapping ourselves, and can rely on the browser to do that for us. The API for all modal components is not changing, but the DOM structure of the Modal components themselves have changed drastically, as well as where they are placed within the DOM itself.
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import React, { useState } from 'react';
import {
act,
fireEvent,
render,
waitForElementToBeRemoved,
} from '@testing-library/react';
import { act, fireEvent, render, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { axe } from 'jest-axe';

Expand All @@ -22,10 +17,12 @@
return (
<ConfirmationModal
title="Title text"
buttonText="Confirm"
confirmButtonProps={{
onClick: () => setOpen(false),
children: 'Confirm',
}}
cancelButtonProps={{ onClick: () => setOpen(false) }}
open={open}
onConfirm={() => setOpen(false)}
onCancel={() => setOpen(false)}
{...props}
>
{props.children ?? 'Content text'}
Expand All @@ -40,6 +37,26 @@
}

describe('packages/confirmation-modal', () => {
beforeAll(() => {
HTMLDialogElement.prototype.show = jest.fn(function mock(
this: HTMLDialogElement,
) {
this.open = true;
});

HTMLDialogElement.prototype.showModal = jest.fn(function mock(
this: HTMLDialogElement,
) {
this.open = true;
});

HTMLDialogElement.prototype.close = jest.fn(function mock(
this: HTMLDialogElement,
) {
this.open = false;
});
});

describe('a11y', () => {
test('does not have basic accessibility issues', async () => {
const { container, getByText } = renderModal({ open: true });
Expand All @@ -55,9 +72,10 @@
});
});

test('does not render if closed', () => {
renderModal();
expect(document.body.innerHTML).toEqual('<div></div>');
test('is not visible when closed', () => {
const { getByRole } = renderModal();
const dialog = getByRole('dialog', { hidden: true });
expect(dialog).not.toBeVisible();
});

test('renders if open', () => {
Expand Down Expand Up @@ -188,21 +206,19 @@
describe('closes when', () => {
test('escape key is pressed', async () => {
const { getByRole } = renderModal({ open: true });
const modal = getByRole('dialog');

fireEvent.keyDown(document, { key: 'Escape', keyCode: 27 });

await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));
});

test('x icon is clicked', async () => {
const { getByLabelText, getByRole } = renderModal({ open: true });
const modal = getByRole('dialog');

const x = getByLabelText('Close modal');
fireEvent.click(x);

await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));
});
});

Expand Down Expand Up @@ -271,7 +287,7 @@
requiredInputText,
});

const modal = getByRole('dialog');

Check warning on line 290 in packages/confirmation-modal/src/ConfirmationModal/ConfirmationModal.spec.tsx

View workflow job for this annotation

GitHub Actions / Check lints

'modal' is assigned a value but never used. Allowed unused vars must match /^_/u
const confirmationButton = await findByTestId(
LGIDS_CONFIRMATION_MODAL.confirm,
);
Expand All @@ -298,7 +314,7 @@

userEvent.click(buttonToClick);

await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));

rerender(
<ConfirmationModal
Expand Down Expand Up @@ -349,13 +365,12 @@
const confirmationButton = getByText('Confirm').closest('button');
expect(confirmationButton).toHaveAttribute('aria-disabled', 'false');

const modal = getByRole('dialog');
const button = getByText('Confirm');
expect(button).toBeVisible();

// Modal doesn't close when button is clicked
fireEvent.click(button);
await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));
});

test('"confirmButtonProps" has "disabled: false"', async () => {
Expand All @@ -369,13 +384,12 @@
const confirmationButton = getByText('Confirm').closest('button');
expect(confirmationButton).toHaveAttribute('aria-disabled', 'false');

const modal = getByRole('dialog');
const button = getByText('Confirm');
expect(button).toBeVisible();

// Modal doesn't close when button is clicked
fireEvent.click(button);
await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
buttonStyle,
contentDarkModeStyles,
contentStyle,
contentVariantStyles,
footerStyle,
textEntryInputStyle,
titleStyle,
warningIconStyle,
Expand All @@ -40,7 +40,7 @@ export const ConfirmationModal = React.forwardRef(
cancelButtonProps = {},
...modalProps
}: ConfirmationModalProps,
forwardRef: React.ForwardedRef<HTMLDivElement | null>,
forwardRef: React.ForwardedRef<HTMLDialogElement | null>,
) => {
const [confirmEnabled, setConfirmEnabled] = useState(!requiredInputText);
const { theme, darkMode } = useDarkMode(darkModeProp);
Expand Down Expand Up @@ -97,13 +97,13 @@ export const ConfirmationModal = React.forwardRef(
return (
<Modal
{...modalProps}
contentClassName={baseModalStyle}
className={baseModalStyle}
setOpen={handleCancel}
darkMode={darkMode}
ref={forwardRef}
>
<div
className={cx(contentStyle, contentVariantStyles[variant], {
className={cx(contentStyle, {
[contentDarkModeStyles]: darkMode,
})}
>
Expand All @@ -125,7 +125,7 @@ export const ConfirmationModal = React.forwardRef(
{children}
{textEntryConfirmation}
</div>
<Footer>
<Footer className={footerStyle}>
<Button
{...confirmButtonProps}
data-testid={LGIDS_CONFIRMATION_MODAL.confirm}
Expand Down
17 changes: 5 additions & 12 deletions packages/confirmation-modal/src/ConfirmationModal/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,13 @@ import { Theme } from '@leafygreen-ui/lib';
import { palette } from '@leafygreen-ui/palette';
import { typeScales } from '@leafygreen-ui/tokens';

import { Variant } from './ConfirmationModal.types';

export const titleStyle = css`
line-height: 32px;
margin-bottom: 10px;
`;

export const baseModalStyle = css`
width: 600px;
padding: initial;
letter-spacing: 0;
`;

Expand All @@ -25,15 +22,6 @@ export const contentDarkModeStyles = css`
color: ${palette.gray.light1};
`;

export const contentVariantStyles: Record<Variant, string> = {
[Variant.Default]: css`
padding: 40px 36px 0px;
`,
[Variant.Danger]: css`
padding: 40px 36px 0px 78px;
`,
};

export const textEntryInputStyle = css`
width: 300px;
margin-top: 14px;
Expand Down Expand Up @@ -79,3 +67,8 @@ export const warningIconThemeStyle: Record<Theme, string> = {
background: ${palette.red.dark2};
`,
};

export const footerStyle = css`
margin-top: 24px;
padding: 0;
`;
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import React, { useState } from 'react';
import {
act,
fireEvent,
render,
waitForElementToBeRemoved,
} from '@testing-library/react';
import { act, fireEvent, render, waitFor } from '@testing-library/react';
import { axe } from 'jest-axe';

import MarketingModal from '..';
Expand Down Expand Up @@ -39,6 +34,26 @@ function renderModal(
}

describe('packages/marketing-modal', () => {
beforeAll(() => {
HTMLDialogElement.prototype.show = jest.fn(function mock(
this: HTMLDialogElement,
) {
this.open = true;
});

HTMLDialogElement.prototype.showModal = jest.fn(function mock(
this: HTMLDialogElement,
) {
this.open = true;
});

HTMLDialogElement.prototype.close = jest.fn(function mock(
this: HTMLDialogElement,
) {
this.open = false;
});
});

describe('a11y', () => {
test('does not have basic accessibility issues', async () => {
const { container, getByText } = renderModal({ open: true });
Expand All @@ -53,13 +68,19 @@ describe('packages/marketing-modal', () => {
expect(newResults).toHaveNoViolations();
});
});
test('does not render if closed', () => {
renderModal();
expect(document.body.innerHTML).toEqual('<div></div>');

test('is not visible when closed', () => {
const { getByRole } = renderModal();
const dialog = getByRole('dialog', { hidden: true });
expect(dialog).not.toBeVisible();
});

test('renders if open', () => {
const { getByText, getByLabelText } = renderModal({ open: true });
test('is visible if open', () => {
const { getByText, getByLabelText, getByRole } = renderModal({
open: true,
});
const dialog = getByRole('dialog');
expect(dialog).toBeVisible();
expect(getByLabelText('Image graphic')).toBeVisible();
expect(getByText('Title text')).toBeVisible();
expect(getByText('Content text')).toBeVisible();
Expand Down Expand Up @@ -112,21 +133,19 @@ describe('packages/marketing-modal', () => {
describe('closes when', () => {
test('escape key is pressed', async () => {
const { getByRole } = renderModal({ open: true });
const modal = getByRole('dialog');

fireEvent.keyDown(document, { key: 'Escape', keyCode: 27 });

await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));
});

test('x icon is clicked', async () => {
const { getByLabelText, getByRole } = renderModal({ open: true });
const modal = getByRole('dialog');

const x = getByLabelText('Close modal');
fireEvent.click(x);

await waitForElementToBeRemoved(modal);
await waitFor(() => getByRole('dialog', { hidden: true }));
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export const titleStyle = css`

export const baseModalStyle = css`
width: 600px;
padding: initial;
padding: unset;
overflow: hidden;
`;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const MarketingModal = ({
return (
<Modal
{...modalProps}
contentClassName={baseModalStyle}
className={baseModalStyle}
setOpen={onClose}
darkMode={darkMode}
closeIconColor={closeIconColor}
Expand Down
Loading