Skip to content

Commit

Permalink
fix: Fix modal causing body padding to increase when unmounted (#1899)
Browse files Browse the repository at this point in the history 
* Fix modal causing body padding to increase when unmounted

* Add docs about testing on a NextJS app
  • Loading branch information
Suzanne Rozier authored Feb 11, 2022
1 parent 6533345 commit fb46e88
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 56 deletions.
45 changes: 43 additions & 2 deletions docs/contributing.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,33 +128,72 @@ You can then run `yarn build:watch` in ReactUSWDS and your application's compile

> **Warning:** Make sure to **unlink** the package once you're done making changes! It can be easy to forget you're using a linked package in your application, and commit changes without switching back to a released version of ReactUSWDS.
> If you get errors about invalid hook calls, you may also need to link the `react` & `react-dom` packages:
>
> ```
> ➜ cd /path/to/react-uswds
> ➜ yarn link
> ➜ yarn install
>
> ➜ cd node_modules/react
> ➜ yarn link
>
> ➜ cd ../../node_modules/react-dom
> ➜ yarn link
>
> ➜ cd /path/to/your/application
> ➜ yarn link @trussworks/react-uswds
> ➜ yarn link react
> ➜ yarn link react-dom
> ```
> If you still get errors about invalid hook calls, and your application is built on NextJS, you may need to add this to your Webpack config (in next.config.js):
>
> ```
> webpack: (config, options) => {
> if (options.isServer) {
> config.externals = ['react', ...config.externals]
> }
>
> config.resolve.alias['react'] = path.resolve(__dirname, '.', 'node_modules', 'react')
>
> return config
> },
> ```
To unlink the package from your application code (you don't need to unlink your local ReactUSWDS code)
:

```
cd /path/to/your/application
➜ yarn unlink @trussworks/react-uswds
yarn unlink v1.22.4
success Removed linked package "@trussworks/react-uswds".
info You will need to run `yarn install --force` to re-install the package that was linked.
➜ yarn install --force
```

#### Install from a ReactUSWDS branch

Another option is to install a specific branch of ReactUSWDS to your application that hasn't been released yet. To do that, you will need to change the reference in your application's `package.json` file:

```
"@trussworks/react-uswds": "https://github.com/trussworks/react-uswds#name-of-branch",
```

After making that change, run `yarn install` to install that version of the package. This will also build the ReactUSWDS code so it can be used in your application as if it were being installed from NPM. You can verify the source of the package by looking in your application's `yarn.lock` file:

```
"@trussworks/react-uswds@https://github.com/trussworks/react-uswds":
version "1.13.2"
resolved "https://github.com/trussworks/react-uswds#92ea5f07f7212370165423ec09ed35eec3aa7e58"
version "1.13.2"
resolved "https://github.com/trussworks/react-uswds#92ea5f07f7212370165423ec09ed35eec3aa7e58"
```

The important thing to note here is that `resolved` is pointing to the Github repo instead of the package registry, and the SHA should match the last commit of the branch you're using.
Expand All @@ -174,8 +213,10 @@ When opening the pull request, it's important to understand the [Conventional Co
The format for PR commits is:

```
<type>(scope): <subject>
<BLANK LINE>
<body>
<BLANK LINE>
<footer>
Expand Down
60 changes: 58 additions & 2 deletions src/components/Modal/Modal.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ const renderWithModalRoot = (
ui: React.ReactElement,
options: RenderOptions = {}
) => {
const appContainer = document.createElement("div")
appContainer.setAttribute("id", "app-root")
const appContainer = document.createElement('div')
appContainer.setAttribute('id', 'app-root')

const modalContainer = document.createElement('div')
modalContainer.setAttribute('id', 'modal-root')
Expand Down Expand Up @@ -649,4 +649,60 @@ describe('Modal component', () => {
})
})
})

describe('unmounting', () => {
it('resets the body element if the modal was open', async () => {
const modalRef = createRef<ModalRef>()
const handleOpen = () => modalRef.current?.toggleModal(undefined, true)

const { baseElement, unmount } = renderWithModalRoot(
<Modal id="testModal" ref={modalRef}>
Test modal
</Modal>
)

expect(baseElement).not.toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 0px')

await waitFor(() => handleOpen())

expect(baseElement).toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 15px')

await waitFor(() => unmount())

expect(baseElement).not.toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 0px')
})

it('does not reset the body element if the modal was not open', async () => {
const modalRef = createRef<ModalRef>()
const handleOpen = () => modalRef.current?.toggleModal(undefined, true)
const handleClose = () => modalRef.current?.toggleModal(undefined, false)

const { baseElement, unmount } = renderWithModalRoot(
<Modal id="testModal" ref={modalRef}>
Test modal
</Modal>
)

expect(baseElement).not.toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 0px')

await waitFor(() => handleOpen())

expect(baseElement).toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 15px')

await waitFor(() => handleClose())

expect(baseElement).not.toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 0px')

await waitFor(() => unmount())

expect(baseElement).not.toHaveClass('usa-js-modal--active')
expect(baseElement).toHaveStyle('padding-right: 0px')
})
})
})
89 changes: 37 additions & 52 deletions src/components/Modal/Modal.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export type ModalRef = {

// Modals are rendered into the document body default. If an element exists with the id
// `modal-root`, that element will be used as the parent instead.
//
//
// If you wish to override this behavior, `renderToPortal` to `false` and the modal
// will render in its normal location in the document. Note that this may cause the modal to
// be inaccessible due to no longer being in the document's accessbility tree.
Expand Down Expand Up @@ -74,6 +74,33 @@ export const Modal = forwardRef(
[id, isOpen]
)

const handleOpenEffect = () => {
const { body } = document
body.style.paddingRight = tempPaddingRef.current || ''
body.classList.add('usa-js-modal--active')

document.querySelectorAll(NON_MODALS).forEach((el) => {
el.setAttribute('aria-hidden', 'true')
el.setAttribute('data-modal-hidden', '')
})

if (forceAction) {
body.classList.add('usa-js-no-click')
}
}

const handleCloseEffect = () => {
const { body } = document
body.style.paddingRight = initialPaddingRef.current || ''
body.classList.remove('usa-js-modal--active')
body.classList.remove('usa-js-no-click')

document.querySelectorAll(NON_MODALS_HIDDEN).forEach((el) => {
el.removeAttribute('aria-hidden')
el.removeAttribute('data-modal-hidden')
})
}

useEffect(() => {
const SCROLLBAR_WIDTH = getScrollbarWidth()
const INITIAL_PADDING =
Expand All @@ -92,57 +119,17 @@ export const Modal = forwardRef(
setMounted(true)

return () => {
// On unmount
const { body } = document
const INITIAL_PADDING = initialPaddingRef.current
const TEMPORARY_PADDING = tempPaddingRef.current

body.classList.remove('usa-js-modal--active')
body.classList.remove('usa-js-no-click')

body.style.paddingRight =
(body.style.paddingRight === TEMPORARY_PADDING
? INITIAL_PADDING
: TEMPORARY_PADDING) || ''

document.querySelectorAll(NON_MODALS_HIDDEN).forEach((el) => {
el.removeAttribute('aria-hidden')
el.removeAttribute('data-modal-hidden')
})
// Reset as if the modal is being closed
handleCloseEffect()
}
}, [])

useEffect(() => {
if (mounted) {
const { body } = document

const INITIAL_PADDING = initialPaddingRef.current
const TEMPORARY_PADDING = tempPaddingRef.current

body.style.paddingRight =
(body.style.paddingRight === TEMPORARY_PADDING
? INITIAL_PADDING
: TEMPORARY_PADDING) || ''

if (isOpen === true) {
body.classList.add('usa-js-modal--active')

document.querySelectorAll(NON_MODALS).forEach((el) => {
el.setAttribute('aria-hidden', 'true')
el.setAttribute('data-modal-hidden', '')
})

if (forceAction) {
body.classList.add('usa-js-no-click')
}
handleOpenEffect()
} else if (isOpen === false) {
body.classList.remove('usa-js-modal--active')
body.classList.remove('usa-js-no-click')

document.querySelectorAll(NON_MODALS_HIDDEN).forEach((el) => {
el.removeAttribute('aria-hidden')
el.removeAttribute('data-modal-hidden')
})
handleCloseEffect()
}
}
}, [isOpen])
Expand Down Expand Up @@ -171,7 +158,7 @@ export const Modal = forwardRef(
},
}

const modal =
const modal = (
<FocusTrap active={isOpen} focusTrapOptions={focusTrapOptions}>
<ModalWrapper
role="dialog"
Expand All @@ -194,14 +181,12 @@ export const Modal = forwardRef(
</ModalWindow>
</ModalWrapper>
</FocusTrap>

)

if (renderToPortal) {
const modalRoot = document.getElementById("modal-root")
const modalRoot = document.getElementById('modal-root')
const target = modalRoot || document.body
return ReactDOM.createPortal(
modal,
target
)
return ReactDOM.createPortal(modal, target)
} else {
return modal
}
Expand Down

0 comments on commit fb46e88

Please sign in to comment.