lmc-eu · crishpeen · May 21, 2024 · May 21, 2024
diff --git a/docs/migrations/web-react/MIGRATION-v2.md b/docs/migrations/web-react/MIGRATION-v2.md
@@ -271,6 +271,9 @@ See [Codemods documentation][readme-codemods] for more details.
 
 Or manually add `isScrollable` prop to the `ModalDialog` component.
 
+If you use `ScrollView` for scrolling the content of your modal, you must also make the
+`ModalDialog` scrollable by setting the `isScrollable` prop.
+
 ### Modal: ModalDialog Uniform Appearance
 
 The uniform `ModalDialog` appearance replaced the current behavior. Current mobile appearance

diff --git a/packages/web-react/DEPRECATIONS-v2.md b/packages/web-react/DEPRECATIONS-v2.md
@@ -50,24 +50,6 @@ Use:
 
 See [`Tooltip` documentation][tooltip-readme] for more details and examples.
 
-### ModalDialog `isScrollable` Prop
-
-The `isScrollable` prop will be set to `false` by default in the next major release and the ModalDialog will be made
-non-scrollable by default. It will be possible to re-enable the inside scrolling by adding the `isScrollable` prop.
-
-#### Migration Guide
-
-Use codemod to automatically update your codebase.
-
-```sh
-npx @lmc-eu/spirit-codemods -p <path> -t v2/web-react/modal-iscrollable-prop
-```
-
-See [Codemods documentation][readme-codemods] for more details.
-
-Or manually add `isScrollable` prop to the `ModalDialog` component.
-
-[readme-codemods]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/codemods/README.md
 [dropdown-readme]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/src/components/Dropdown/README.md
 [readme-deprecations]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/README.md#deprecations
 [tooltip-readme]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/src/components/Tooltip/README.md
diff --git a/packages/web-react/src/components/Modal/README.md b/packages/web-react/src/components/Modal/README.md
@@ -13,8 +13,6 @@ Modal is a composition of several subcomponents:
     - [Dropdowns in Modal](#dropdowns-in-modal)
     - [Docked Modals on Mobile Screens](#docked-modals-on-mobile-screens)
       - [Expanded Variant](#expanded-variant)
-    - [Custom Height](#custom-height)
-    - [Custom Max Height](#custom-max-height)
     - [API](#api-1)
   - [ModalHeader](#modalheader)
     - [Hidden Title](#hidden-title)
@@ -27,8 +25,10 @@ Modal is a composition of several subcomponents:
     - [API](#api-4)
   - [Opening the Modal](#opening-the-modal)
   - [Scrolling Long Content](#scrolling-long-content)
+    - [Enable Scrolling Inside ModalDialog](#enable-scrolling-inside-modaldialog)
     - [Scrolling with ScrollView](#scrolling-with-scrollview)
-    - [Disable Scrolling Inside ModalDialog](#disable-scrolling-inside-modaldialog)
+    - [Custom Height](#custom-height)
+    - [Custom Max Height](#custom-max-height)
   - [Stacking Modals](#stacking-modals)
   - [Full Example](#full-example)
 
@@ -112,7 +112,7 @@ On mobile screens, Modal can be docked to the bottom of the viewport using the `
 
 #### Expanded Variant
 
-By default the docked dialog on mobile screens shrinks to fit the height of its content
+By default, the docked dialog on mobile screens shrinks to fit the height of its content
 (if smaller than the viewport). Use the `isExpandedOnMobile` option to expand the dialog on mobile.
 
 ```jsx
@@ -121,56 +121,18 @@ By default the docked dialog on mobile screens shrinks to fit the height of its
 </ModalDialog>
 ```
 
-### Custom Height
-
-By default, Modal expands to fit the height of its content, as long as it fits the viewport (see [more below](#custom-max-height)).
-You can override this behavior by setting a preferred height using the following options:
-
-- `preferredHeightOnMobile` for mobile screens, and
-- `preferredHeightFromTabletUp` for tablet screens and up.
-
-This is useful for Modals with dynamic content, e.g. a list of items that can be added or removed, or a multistep wizard.
-
-```jsx
-<ModalDialog preferredHeightOnMobile="400px" preferredHeightFromTabletUp="500px">
-  …
-</ModalDialog>
-```
-
-👉 Please note the preferred height options are ignored when scrolling inside ModalDialog is
-[turned off](#disable-scrolling-inside-modaldialog).
-
-👉 Please note the custom height values are considered **preferred:** Modal will not expand beyond the viewport height.
-
-### Custom Max Height
-
-The default maximum height of Modal is:
-
-- viewport height minus 1100 spacing on mobile screens, and
-- 600 px on tablet screens and up (shrunk if necessary).
-
-You can use the `maxHeightFromTabletUp` option to override the max height on tablet screens and up:
-
-```jsx
-<ModalDialog maxHeightFromTabletUp="700px">…</ModalDialog>
-```
-
-👉 Please note the max height is ignored when scrolling inside ModalDialog is [turned off](#disable-scrolling-inside-modaldialog).
-
-👉 Please note the max height on mobile screens is currently not customizable. Let us know if you need this feature! 🙏
-
 ### API
 
-| Name                          | Type                  | Default   | Required | Description                                                                                                                              |
-| ----------------------------- | --------------------- | --------- | -------- | ---------------------------------------------------------------------------------------------------------------------------------------- |
-| `children`                    | `ReactNode`           | —         | ✕        | Children node                                                                                                                            |
-| `elementType`                 | [`article` \| `form`] | `article` | ✕        | ModalDialog element type                                                                                                                 |
-| `isDockedOnMobile`            | `bool`                | `false`   | ✕        | [REQUIRES FEATURE FLAG](#feature-flag-uniform-appearance-on-all-breakpoints): Dock the ModalDialog to the bottom of the screen on mobile |
-| `isExpandedOnMobile`          | `bool`                | `false`   | ✕        | If true, ModalDialog expands to fit the viewport on mobile                                                                               |
-| `isScrollable`                | `bool`                | `true`    | ✕        | If the ModalDialog should be scrollable. If set to `false`, the dialog will not scroll and will expand to fit the content                |
-| `maxHeightFromTabletUp`       | `string`              | `null`    | ✕        | Max height of the modal. Accepts any valid CSS value                                                                                     |
-| `preferredHeightFromTabletUp` | `string`              | `null`    | ✕        | Preferred height of the modal on tablet and larger. Accepts any valid CSS value                                                          |
-| `preferredHeightOnMobile`     | `string`              | `null`    | ✕        | Preferred height of the modal on mobile. Accepts any valid CSS value                                                                     |
+| Name                          | Type                  | Default   | Required | Description                                                                                                               |
+| ----------------------------- | --------------------- | --------- | -------- | ------------------------------------------------------------------------------------------------------------------------- |
+| `children`                    | `ReactNode`           | —         | ✕        | Children node                                                                                                             |
+| `elementType`                 | [`article` \| `form`] | `article` | ✕        | ModalDialog element type                                                                                                  |
+| `isDockedOnMobile`            | `bool`                | `false`   | ✕        | Dock the ModalDialog to the bottom of the screen on mobile                                                                |
+| `isExpandedOnMobile`          | `bool`                | `false`   | ✕        | If true, ModalDialog expands to fit the viewport on mobile                                                                |
+| `isScrollable`                | `bool`                | `true`    | ✕        | If the ModalDialog should be scrollable. If set to `false`, the dialog will not scroll and will expand to fit the content |
+| `maxHeightFromTabletUp`       | `string`              | `null`    | ✕        | Max height of the modal. Accepts any valid CSS value                                                                      |
+| `preferredHeightFromTabletUp` | `string`              | `null`    | ✕        | Preferred height of the modal on tablet and larger. Accepts any valid CSS value                                           |
+| `preferredHeightOnMobile`     | `string`              | `null`    | ✕        | Preferred height of the modal on mobile. Accepts any valid CSS value                                                      |
 
 Also, all properties of the [`<article>` element][mdn-article] and [`<form>` element][mdn-form] are supported.
 
@@ -308,38 +270,75 @@ const handleClose = () => setOpen(false);
 
 ## Scrolling Long Content
 
-When Modals become too long for the user's viewport or device, they automatically scroll independently of the page itself.
+In case the content is longer than user's viewport or device, the ModalBody will expand to fit the height of its content
+and the whole ModalDialog will scroll.
+
+### Enable Scrolling Inside ModalDialog
+
+Scrolling inside ModalDialog can be turned on by adding the `isScrollable` prop:
+
+```jsx
+<ModalDialog isScrollable>…</ModalDialog>
+```
 
 ### Scrolling with ScrollView
 
 To make content overflow more obvious to users, you can wrap the `ModalBody` content in a [ScrollView][scroll-view] that
 takes over the responsibility for scrolling and provides visual overflow decorators, e.g.:
 
 ```jsx
-<ScrollView overflowDecorators="both">
-  <ModalBody>…Long content…</ModalBody>
-</ScrollView>
+<ModalDialog isScrollable>
+  …
+  <ScrollView overflowDecorators="both">
+    <ModalBody>…Long content…</ModalBody>
+  </ScrollView>
+  …
+</ModalDialog>
 ```
 
-### Disable Scrolling Inside ModalDialog
+### Custom Height
+
+By default, ModalDialog grows to the height of its content until it reaches the [maximum height](#custom-max-height)
+limit.
+
+You can set a custom preferred height of ModalDialog using a custom property:
+
+- `preferredHeightOnMobile` for mobile screens, and
+- `preferredHeightFromTabletUp` for tablet screens and up.
 
-Scrolling inside ModalDialog can be turned off by setting the `ModalDialog` prop `isScrollable` to `false`:
+This is useful for Modals with dynamic content, e.g. a list of items that can be added or removed, or a multistep wizard.
 
 ```jsx
-<ModalDialog isScrollable="false">
-  <!-- … -->
+<ModalDialog isScrollable preferredHeightOnMobile="400px" preferredHeightFromTabletUp="500px">
+  …
 </ModalDialog>
 ```
 
-This way, the ModalBody will expand to fit the height of its content and the whole ModalDialog will scroll in case the
-content is longer than user's viewport.
+⚠️ This feature is only available for ModalDialogs with the `isScrollable` prop.
+
+👉 Please note the custom height values are considered **preferred**. Scrollable ModalDialog will never expand beyond
+the viewport height. See the [Custom Max Height](#custom-max-height) section for more information.
+
+### Custom Max Height
+
+The default maximum height of a scrollable ModalDialog is **600 px**, as long as it can fit the viewport.
 
-👉 Please note that this modifier class can produce unexpected results when used in combination with ScrollView.
+If the viewport is smaller, scrollable ModalDialog will shrink to fit the viewport. In such case, the ModalDialog height
+will calculate as "viewport height (`100dvh`) minus 1100 spacing".
 
-#### ⚠️ DEPRECATION NOTICE
+You can use the prop `maxHeightFromTabletUp` to override the default maximum height limit on tablet
+screens and up:
 
-The `isScrollable` prop will be set to `false` by default in the next major release and the ModalDialog will be made
-non-scrollable by default. It will be possible to re-enable the inside scrolling by adding the `isScrollable` prop.
+```jsx
+<ModalDialog isScrollable maxHeightFromTabletUp="700px">
+  …
+</ModalDialog>
+```
+
+⚠️ This feature is only available for ModalDialogs with the `isScrollable` prop.
+
+👉 If a [custom height](#custom-height) is set, the custom max height is only applied if it's smaller than the custom
+height.
 
 ## Stacking Modals
 
@@ -400,17 +399,13 @@ const handleClose = () => setOpen(false);
 </Modal>
 ```
 
-[What are deprecations?][readme-deprecations]
-
 [dictionary-alignment]: https://github.com/lmc-eu/spirit-design-system/blob/main/docs/DICTIONARIES.md#alignment
 [mdn-article]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/article
 [mdn-dialog-form]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#usage_notes
 [mdn-dialog]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog
 [mdn-form]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/form
 [modal]: https://github.com/lmc-eu/spirit-design-system/tree/main/packages/web/src/scss/components/Modal
 [readme-additional-attributes]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/README.md#additional-attributes
-[readme-deprecations]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/README.md#deprecations
 [readme-escape-hatches]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/README.md#escape-hatches
-[readme-feature-flags]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web/README.md#feature-flags
 [readme-style-props]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/README.md#style-props
 [scroll-view]: https://github.com/lmc-eu/spirit-design-system/blob/main/packages/web-react/src/components/ScrollView/README.md
diff --git a/packages/web-react/src/components/Modal/__tests__/Modal.test.tsx b/packages/web-react/src/components/Modal/__tests__/Modal.test.tsx
@@ -1,6 +1,6 @@
 import '@testing-library/jest-dom';
 import React from 'react';
-import { fireEvent, render } from '@testing-library/react';
+import { fireEvent, render, screen } from '@testing-library/react';
 import { classNamePrefixProviderTest } from '../../../../tests/providerTests/classNamePrefixProviderTest';
 import { stylePropsTest } from '../../../../tests/providerTests/stylePropsTest';
 import { restPropsTest } from '../../../../tests/providerTests/restPropsTest';
@@ -22,27 +22,27 @@ describe('Modal', () => {
 
   it('should not close modal dialog', () => {
     const mockedOnClose = jest.fn();
-    const dom = render(
+    render(
       <Modal id="test" isOpen onClose={mockedOnClose} closeOnBackdropClick={false}>
         <div>Test</div>
       </Modal>,
     );
 
-    const dialog = dom.container.querySelector('.Modal') as HTMLElement;
+    const dialog = screen.getByRole('dialog');
     fireEvent.click(dialog);
 
     expect(mockedOnClose).not.toHaveBeenCalled();
   });
 
   it('should close modal dialog', () => {
     const mockedOnClose = jest.fn();
-    const dom = render(
+    render(
       <Modal id="test" isOpen onClose={mockedOnClose} closeOnBackdropClick>
         <div>Test</div>
       </Modal>,
     );
 
-    const dialog = dom.container.querySelector('.Modal') as HTMLElement;
+    const dialog = screen.getByRole('dialog');
     fireEvent.click(dialog);
 
     expect(mockedOnClose).toHaveBeenCalled();

diff --git a/packages/web-react/src/components/Modal/__tests__/ModalCloseButton.test.tsx b/packages/web-react/src/components/Modal/__tests__/ModalCloseButton.test.tsx
@@ -1,5 +1,5 @@
 import '@testing-library/jest-dom';
-import { fireEvent, render } from '@testing-library/react';
+import { fireEvent, render, screen } from '@testing-library/react';
 import React from 'react';
 import { classNamePrefixProviderTest } from '../../../../tests/providerTests/classNamePrefixProviderTest';
 import { restPropsTest } from '../../../../tests/providerTests/restPropsTest';
@@ -14,35 +14,39 @@ describe('ModalCloseButton', () => {
   restPropsTest(ModalCloseButton, 'button');
 
   it('should have icon', () => {
-    const dom = render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
+    render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
 
-    expect(dom.container.querySelector('svg')).toBeDefined();
+    const svg = screen.getByRole('button').firstElementChild;
+
+    expect(svg).toBeInTheDocument();
+    expect(svg).toHaveAttribute('aria-hidden', 'true');
   });
 
   it('should have visually hidden label', () => {
-    const dom = render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
+    render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
 
-    expect(dom.container.querySelector('button > span')?.textContent).toBe('close');
-    expect(dom.container.querySelector('button > span')).toHaveClass('accessibility-hidden');
+    const buttonText = screen.getByRole('button').lastElementChild;
+    expect(buttonText?.textContent).toBe('close');
+    expect(buttonText).toHaveClass('accessibility-hidden');
   });
 
   it('should be square', () => {
-    const dom = render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
+    render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
 
-    expect(dom.container.querySelector('button')).toHaveClass('Button--square');
+    expect(screen.getByRole('button')).toHaveClass('Button--square');
   });
 
   it('should have tertiary color', () => {
-    const dom = render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
+    render(<ModalCloseButton label="close" id="test" isOpen onClose={() => {}} />);
 
-    expect(dom.container.querySelector('button')).toHaveClass('Button--tertiary');
+    expect(screen.getByRole('button')).toHaveClass('Button--tertiary');
   });
 
   it('should handle on close click', () => {
     const mockedOnClose = jest.fn();
-    const dom = render(<ModalCloseButton label="close" id="test" isOpen onClose={mockedOnClose} />);
+    render(<ModalCloseButton label="close" id="test" isOpen onClose={mockedOnClose} />);
 
-    const button = dom.getByRole('button');
+    const button = screen.getByRole('button');
     fireEvent.click(button);
 
     expect(mockedOnClose).toHaveBeenCalled();