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

Re-implement Modal component using HTMLDialogElement (#461) #544

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
43 changes: 22 additions & 21 deletions src/components/Modal/Modal.jsx
Copy link
Member

@adamkudrna adamkudrna Jun 17, 2024

Choose a reason for hiding this comment

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

obrazek

I'd expect more deletions than additions. 😞 Can we get rid of some more features custom code in our focus management? Or anywhere else?

Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import PropTypes from 'prop-types';
import React, { useRef } from 'react';
import React, {
useEffect,
useRef,
} from 'react';
import { createPortal } from 'react-dom';
import { withGlobalProps } from '../../provider';
import { transferProps } from '../_helpers/transferProps';
Expand All @@ -18,32 +21,27 @@ const preRender = (
restProps,
size,
) => (
<div
className={styles.backdrop}
<dialog
{...transferProps(restProps)}
className={classNames(
styles.root,
getSizeClassName(size, styles),
getPositionClassName(position, styles),
)}
onClick={(e) => {
e.stopPropagation();
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
}}
onClose={(e) => {
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
e.preventDefault();
if (closeButtonRef?.current != null) {
closeButtonRef.current.click();
}
}}
ref={childrenWrapperRef}
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
role="presentation"
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
>
<div
{...transferProps(restProps)}
className={classNames(
styles.root,
getSizeClassName(size, styles),
getPositionClassName(position, styles),
)}
onClick={(e) => {
e.stopPropagation();
}}
ref={childrenWrapperRef}
role="presentation"
>
{children}
</div>
</div>
{children}
</dialog>
);

export const Modal = ({
Expand All @@ -59,11 +57,14 @@ export const Modal = ({
}) => {
const childrenWrapperRef = useRef();

useEffect(() => {
childrenWrapperRef.current.showModal();
}, []);

useModalFocus(
autoFocus,
childrenWrapperRef,
primaryButtonRef,
closeButtonRef,
);

useModalScrollPrevention(preventScrollUnderneath);
Expand Down Expand Up @@ -121,7 +122,7 @@ Modal.propTypes = {
*/
children: PropTypes.node,
/**
* Reference to close button element. It is used to close modal when Escape key is pressed or the backdrop is clicked.
* Reference to close button element. It is used to close modal when Escape key is pressed.
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
*/
closeButtonRef: PropTypes.shape({
// eslint-disable-next-line react/forbid-prop-types
Expand Down
37 changes: 20 additions & 17 deletions src/components/Modal/Modal.module.scss
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
// 1. Modal uses <dialog> element that uses the browser's built-in dialog functionality, so that:
// * visibility of thw .root element and its backdrop is managed by the browser
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
// * positioning of the .root element and its backdrop is managed by the browser
// * z-index of the .root element and its backdrop is not needed as dialog is rendered in browser's Top layer
// 2. When dialogs are stacked, only the topmost dialog should have a backdrop.
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved

@use "sass:map";
@use "../../styles/theme/typography";
@use "../../styles/tools/accessibility";
@use "../../styles/tools/breakpoint";
@use "../../styles/tools/reset";
@use "../../styles/tools/spacing";
@use "animations";
@use "settings";
@use "theme";

Expand All @@ -13,33 +20,33 @@
--rui-local-max-width: calc(100% - (2 * var(--rui-local-outer-spacing)));
--rui-local-max-height: calc(100% - (2 * var(--rui-local-outer-spacing)));

position: fixed;
left: 50%;
z-index: settings.$z-index;
display: flex;
flex-direction: column;
max-width: var(--rui-local-max-width);
max-height: var(--rui-local-max-height);
padding: 0;
overflow-y: auto;
overscroll-behavior: contain;
border-radius: settings.$border-radius;
background: theme.$background;
box-shadow: theme.$box-shadow;
transform: translateX(-50%);

@include breakpoint.up(sm) {
--rui-local-outer-spacing: #{theme.$outer-spacing-sm};
}
}

.backdrop {
position: fixed;
top: 0;
left: 0;
z-index: settings.$backdrop-z-index;
width: 100vw;
height: 100vh;
.root:has(.root)::backdrop {
Copy link
Member

@adamkudrna adamkudrna Jun 17, 2024

Choose a reason for hiding this comment

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

⚠️ <dialog>s should rather not be nested like that. It is OK to stack <dialog>s visually, but in practice, it should mean they are siblings in the DOM. While the HTML spec does not explicitly discourage from nesting one <dialog> into another in the DOM (and while it seems to "work" somehow), we (me and the Spirit team) are not quite sure of the consequences.

OK:

<dialog></dialog>
<dialog></dialog>

⚠️ Not OK:

<dialog>
  <dialog></dialog>
</dialog>

https://github.com/lmc-eu/spirit-design-system/tree/main/packages/web-react/src/components/Modal#stacking-modals

background: transparent; // 2.
}

.root[open] {
display: flex;
animation: fade-in theme.$animation-duration ease-out;
}

.root[open]::backdrop {
background: theme.$backdrop-background;
animation: fade-in theme.$animation-duration ease-out;
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
}

.isRootSizeSmall {
Expand Down Expand Up @@ -69,12 +76,8 @@
max-width: min(var(--rui-local-max-width), #{map.get(theme.$sizes, auto, max-width)});
}

.isRootPositionCenter {
top: 50%;
transform: translate(-50%, -50%);
}

.isRootPositionTop {
position: sticky;
top: var(--rui-local-outer-spacing);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should do the job:

Suggested change
position: sticky;
top: var(--rui-local-outer-spacing);
top: var(--rui-local-outer-spacing);
bottom: auto;

}
}
7 changes: 5 additions & 2 deletions src/components/Modal/__tests__/Modal.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ import { ModalContent } from '../ModalContent';
import { ModalFooter } from '../ModalFooter';
import { ModalHeader } from '../ModalHeader';

describe('rendering', () => {
// Test suites skipped due tu missing implementation of HTMLDialogElement in jsdom
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
// See https://github.com/jsdom/jsdom/issues/3294

describe.skip('rendering', () => {
it('renders with "portalId" props', () => {
document.body.innerHTML = '<div id="portal-id" />';
render((
Expand Down Expand Up @@ -74,7 +77,7 @@ describe('rendering', () => {
});
});

describe('functionality', () => {
describe.skip('functionality', () => {
it.each([
() => userEvent.keyboard('{Escape}'),
() => userEvent.click(screen.getByTestId('id').parentNode),
Expand Down
9 changes: 9 additions & 0 deletions src/components/Modal/_animations.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@keyframes fade-in {
0% {
opacity: 0;
}

100% {
opacity: 1;
}
}
2 changes: 1 addition & 1 deletion src/components/Modal/_helpers/getPositionClassName.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ export const getPositionClassName = (modalPosition, styles) => {
return styles.isRootPositionTop;
}

return styles.isRootPositionCenter;
return null;
};
7 changes: 0 additions & 7 deletions src/components/Modal/_hooks/useModalFocus.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ export const useModalFocus = (
autoFocus,
childrenWrapperRef,
primaryButtonRef,
closeButtonRef,
) => {
useEffect(
() => {
Expand Down Expand Up @@ -53,11 +52,6 @@ export const useModalFocus = (
};

const keyPressHandler = (e) => {
if (e.key === 'Escape' && closeButtonRef?.current != null) {
closeButtonRef.current.click();
return;
}

if (
e.key === 'Enter'
&& e.target.nodeName !== 'BUTTON'
Expand Down Expand Up @@ -120,7 +114,6 @@ export const useModalFocus = (
autoFocus,
childrenWrapperRef,
primaryButtonRef,
closeButtonRef,
],
);
};
3 changes: 0 additions & 3 deletions src/components/Modal/_settings.scss
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
@use "sass:map";
@use "../../styles/settings/z-indexes";
@use "../../styles/theme/borders";
@use "../../styles/theme/typography";

$border-radius: borders.$radius-2;
$z-index: z-indexes.$modal;
$backdrop-z-index: z-indexes.$modal-backdrop;
$title-font-size: map.get(typography.$font-size-values, 2);
1 change: 1 addition & 0 deletions src/components/Modal/_theme.scss
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ $footer-gap: var(--rui-Modal__footer__gap);
$backdrop-background: var(--rui-Modal__backdrop__background);
$outer-spacing-xs: var(--rui-Modal__outer-spacing--xs);
$outer-spacing-sm: var(--rui-Modal__outer-spacing--sm);
$animation-duration: var(--rui-Modal__animation__duration);

$sizes: (
auto: (
Expand Down
2 changes: 0 additions & 2 deletions src/styles/settings/_z-indexes.scss

This file was deleted.

1 change: 1 addition & 0 deletions src/theme.scss
bedrich-schindler marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,7 @@
--rui-Modal--large__width: 60rem;
--rui-Modal--fullscreen__width: 100%;
--rui-Modal--fullscreen__height: 100%;
--rui-Modal__animation__duration: 0.25s;

//
// Paper
Expand Down