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

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

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
36 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
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 Down Expand Up @@ -40,6 +35,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 +70,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 +204,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 +285,7 @@
requiredInputText,
});

const modal = getByRole('dialog');

Check warning on line 288 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 +312,7 @@

userEvent.click(buttonToClick);

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

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

const modal = getByRole('dialog');

Check warning on line 366 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 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 +383,13 @@
const confirmationButton = getByText('Confirm').closest('button');
expect(confirmationButton).toHaveAttribute('aria-disabled', 'false');

const modal = getByRole('dialog');

Check warning on line 386 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 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 @@ -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
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
71 changes: 32 additions & 39 deletions packages/modal/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,48 +37,40 @@ function ExampleComponent() {
}
```

**Output HTML**

```html
<button>Open Modal</button>
<div aria-modal="true" role="dialog" tabindex="-1" class="leafygreen-ui-2e4yhj">
<button
tabindex="0"
aria-disabled="false"
aria-label="Close modal"
class="leafygreen-ui-zndd6x"
>
<div class="leafygreen-ui-xhlipt">
<svg
class="leafygreen-ui-19fdo3o"
height="20"
width="20"
viewBox="0 0 16 16"
role="img"
>
<g
id="X-Copy"
stroke="none"
stroke-width="1"
fill="none"
fill-rule="evenodd"
>
<path
d="M9,7 L13.5,7 L13.5,9 L9,9 L9,13.5 L7,13.5 L7,9 L2.5,9 L2.5,7 L7,7 L7,2.5 L9,2.5 L9,7 Z"
id="Combined-Shape-Copy"
fill="currentColor"
transform="translate(8.000000, 8.000000) rotate(45.000000) translate(-8.000000, -8.000000) "
></path>
</g>
</svg>
</div></button
>Modal Content goes here.
</div>
## Notes

Portaled Elements within a Modal

Recommended Approach: `renderMode="top-layer"`
When using any LeafyGreen components that wrap a Portal or Popover component, we recommend setting the `renderMode` prop to the value "top-layer". This is the browser's default way of handling hierarchy, without worrying about the DOM or z-index collisions.

Example:

```js
<Modal>
<Select renderMode="top-layer" />
</Modal>
```

## Notes
Fallback:
If for some reason you must use a Portal with an element rendered inside of a Modal component, you can access the Modal's DOM structure by passing a ref to the `portalRef` property in the Modal component. We will set the current value of your ref to an element inside of the Modal itself.

Example:

It is HIGHLY encouraged that any children inside of `Modal` should refrain from using `usePortal={false}` because this can cause stacking context/z-indexing issues since the popover element will render relative to the parent rather than rendering in a `React portal` which is automatically appended to the `Modal`. By default any component that can use a `React portal`, like `Tooltip` or `Select`, will have `usePortal` set to `true`.
```js
const portalRef = useRef<HTMLDivElement | null>(null)
const [containerState, setContainerState] = useState<React.RefObject<HTMLDivElement>>(null)

useEffect(() => {
if (portalRef.current) {
setContainerState(portalRef)
}
}, [portalRef])

<Modal portalRef={portalRef}>
<Select renderMode="portal" portalContainer={containerState.current}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we continue using a PortalContextProvider in Modal.tsx, it should improve the consumer experience since they won't need to specify portalContainer in each popover instance that uses renderMode="portal"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to what I think makes sense but let me know

</Modal>
```

## Properties

Expand All @@ -94,6 +86,7 @@ It is HIGHLY encouraged that any children inside of `Modal` should refrain from
| `initialFocus` | `string` | A selector string for the element to move focus to when the modal is opened. The first focusable element in the modal will receive focus by default. | |
| `darkMode` | `boolean` | Determines if the component will appear in dark mode. | `false` |
| `closeIconColor` | `'default'`, `'dark'`, `'light'` | Determines the color of the close icon. | `default` |
| `portalRef` | `React.RefObject<HTMLDivElement>` | Current property gets set with an element inside the Modal, in order to safely portal elements inside of the dialog element | |

## Using `Clipboard.js` inside `Modal`

Expand Down
6 changes: 2 additions & 4 deletions packages/modal/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,20 @@
"access": "public"
},
"dependencies": {
"@leafygreen-ui/button": "workspace:^",
"@leafygreen-ui/emotion": "workspace:^",
"@leafygreen-ui/hooks": "workspace:^",
"@leafygreen-ui/icon": "workspace:^",
"@leafygreen-ui/icon-button": "workspace:^",
"@leafygreen-ui/lib": "workspace:^",
"@leafygreen-ui/palette": "workspace:^",
"@leafygreen-ui/portal": "workspace:^",
"@leafygreen-ui/tokens": "workspace:^",
"focus-trap": "6.9.4",
"focus-trap-react": "^9.0.2",
"@leafygreen-ui/tooltip": "workspace:^",
"polished": "^4.2.2",
"react-transition-group": "^4.4.5"
},
"devDependencies": {
"@faker-js/faker": "8.0.2",
"@leafygreen-ui/button": "workspace:^",
"@leafygreen-ui/code": "workspace:^",
"@leafygreen-ui/copyable": "workspace:^",
"@leafygreen-ui/select": "workspace:^",
Expand Down
31 changes: 31 additions & 0 deletions packages/modal/src/CloseButton/CloseButton.styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { css } from '@leafygreen-ui/emotion';
import { Theme } from '@leafygreen-ui/lib';
import { palette } from '@leafygreen-ui/palette';

import { CloseIconColor } from '../Modal/Modal.types';

const getColor = (theme: Theme, customColor: CloseIconColor) => {
switch (customColor) {
case 'dark':
return palette.black;

case 'light':
return palette.gray.light2;

default:
return theme === Theme.Light ? palette.gray.dark1 : palette.gray.light2;
}
};

export const closeButtonStyles = (
theme: Theme,
customColor: CloseIconColor,
) => css`
position: absolute;
cursor: pointer;
// x-icon should be 24px from edge. IconButton is 28x28 and Icon is 16x16
// so there's already (28 - 16) / 2 = 6px of spacing. 24 - 6 = 18.
right: 18px;
top: 18px;
color: ${getColor(theme, customColor)};
`;
32 changes: 32 additions & 0 deletions packages/modal/src/CloseButton/CloseButton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react';

import { useIdAllocator } from '@leafygreen-ui/hooks';
import XIcon from '@leafygreen-ui/icon/dist/X';
import IconButton from '@leafygreen-ui/icon-button';
import { useDarkMode } from '@leafygreen-ui/leafygreen-provider';

import { LGIDS_MODAL } from '../constants';
import { CloseIconColor } from '../Modal/Modal.types';

import { closeButtonStyles } from './CloseButton.styles';
import { CloseButtonProps } from './CloseButton.types';

export default function CloseButton({
closeIconColor = CloseIconColor.Default,
handleClose,
}: CloseButtonProps) {
const { theme } = useDarkMode();
const closeId = useIdAllocator({ prefix: 'modal' });

return (
<IconButton
id={closeId}
data-testid={LGIDS_MODAL.close}
onClick={handleClose}
aria-label="Close modal"
className={closeButtonStyles(theme, closeIconColor)}
>
<XIcon />
</IconButton>
);
}
6 changes: 6 additions & 0 deletions packages/modal/src/CloseButton/CloseButton.types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { CloseIconColor } from '../Modal/Modal.types';

export interface CloseButtonProps {
closeIconColor?: CloseIconColor;
handleClose?: () => void;
}
1 change: 1 addition & 0 deletions packages/modal/src/CloseButton/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './CloseButton';
Loading