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.

Sign up for GitHub

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

Fix modal dismissers in development mode #64132

Merged
merged 3 commits into from
Jul 31, 2024
Merged

Fix modal dismissers in development mode #64132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73d437b
Fix modal dismissers in development mode
jsnajdr Jul 31, 2024
96402ab
Keep the close() calls
jsnajdr Jul 31, 2024
d13130d
Add changelog entry
jsnajdr Jul 31, 2024
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- `ColorPalette`: Remove extra bottom margin when `CircularOptionPicker` is unneeded ([#63961](https://github.com/WordPress/gutenberg/pull/63961)).
- `CustomSelectControl`: Restore `describedBy` functionality ([#63957](https://github.com/WordPress/gutenberg/pull/63957)).
- `Modal`: Fix the dismissal logic for React development mode ([#64132](https://github.com/WordPress/gutenberg/pull/64132)).

### Enhancements

Expand Down
41 changes: 21 additions & 20 deletions packages/components/src/modal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,7 @@
* External dependencies
*/
import clsx from 'clsx';
import type {
ForwardedRef,
KeyboardEvent,
MutableRefObject,
UIEvent,
} from 'react';
import type { ForwardedRef, KeyboardEvent, RefObject, UIEvent } from 'react';

/**
* WordPress dependencies
Expand Down Expand Up @@ -44,9 +39,10 @@ import type { ModalProps } from './types';
import { withIgnoreIMEEvents } from '../utils/with-ignore-ime-events';

// Used to track and dismiss the prior modal when another opens unless nested.
const ModalContext = createContext<
MutableRefObject< ModalProps[ 'onRequestClose' ] | undefined >[]
>( [] );
type Dismissers = Set<
RefObject< ModalProps[ 'onRequestClose' ] | undefined >
>;
const ModalContext = createContext< Dismissers >( new Set() );

// Used to track body class names applied while modals are open.
const bodyOpenClasses = new Map< string, number >();
Expand Down Expand Up @@ -147,23 +143,28 @@ function UnforwardedModal(
// one should remain open at a time and the list enables closing prior ones.
const dismissers = useContext( ModalContext );
// Used for the tracking and dismissing any nested modals.
const nestedDismissers = useRef< typeof dismissers >( [] );
const [ nestedDismissers ] = useState< Dismissers >( () => new Set() );

// Updates the stack tracking open modals at this level and calls
// onRequestClose for any prior and/or nested modals as applicable.
useEffect( () => {
dismissers.push( refOnRequestClose );
const [ first, second ] = dismissers;
if ( second ) {
first?.current?.();
// add this modal instance to the dismissers set
dismissers.add( refOnRequestClose );
// request that all the other modals close themselves
for ( const dismisser of dismissers ) {
if ( dismisser !== refOnRequestClose ) {
dismisser.current?.();
}
}

const nested = nestedDismissers.current;
return () => {
nested[ 0 ]?.current?.();
dismissers.shift();
// request that all the nested modals close themselves
for ( const dismisser of nestedDismissers ) {
dismisser.current?.();
}
// remove this modal instance from the dismissers set
dismissers.delete( refOnRequestClose );
};
}, [ dismissers ] );
}, [ dismissers, nestedDismissers ] );

// Adds/removes the value of bodyOpenClassName to body element.
useEffect( () => {
Expand Down Expand Up @@ -350,7 +351,7 @@ function UnforwardedModal(
);

return createPortal(
<ModalContext.Provider value={ nestedDismissers.current }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose avoiding this reading of the ref during render was the idea behind using state instead. Or was it something else?

Copy link
Member

Choose a reason for hiding this comment

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

The state is also often used for one-time initializations. See https://tkdodo.eu/blog/use-state-for-one-time-initializations.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main advantage of useState for one-time initialization like this is that it supports an initialization function. You can write:

useState( () => new Set() );

and avoid creating a new set on each render. It's created only when useState really wants to init and calls the initializer. On the other hand, with useRef you can only do this:

useRef( new Set() );

In this particular case it doesn't matter much, but for more expensive initializations it can make a difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense. Indeed it would hardly matter here and probably neither would there be a consequence to reading the ref in this case. All around it’s better though. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

The useState initialization pattern has been becoming popular recently because of React 19 and the React Compiler: I don't remember the exact details right now, but the compiler issues warnings about some useRef usages, and switching to useState makes it happy.

<ModalContext.Provider value={ nestedDismissers }>
{ modal }
</ModalContext.Provider>,
document.body
Expand Down
Loading