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

Modal: Improve application of body class names #55430

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Oct 17, 2023

What?

Fixes up some obscure bugginess of the bodyOpenClassName prop and adds a couple unit tests to specify the expected behavior.

Why?

To better specify the behavior and avoid potential future issues. It doesn't seem likely this is biting anyone but it could. I found this while working on #51602.

How?

Updates the logic to account for the fact that value of the prop may differ between subsequent modals and could potentially even be changed while a modal is open.

Testing Instructions

Manually

Snippet for manual testing in the Post editor
( ( {
	components: {
		Button,
		Modal,
	},
	editPost: { PluginDocumentSettingPanel },
	element: { createElement: el, Fragment, useReducer, useState },
	plugins: { registerPlugin }
} ) => {
	registerPlugin( 'modal-nesting-demo', {
		render: () => el( PluginDocumentSettingPanel, {
			name: 'modal-demo',
			title: 'Modal demo',
			children: el( ModalNesting )
		} ),
	} );

	const cyclicReducer = ( { value, index, list: heldList }, list = heldList ) => {
		const nextIndex = ( index + 1 ) % list.length;
		return { value: list[ nextIndex ], index: nextIndex, list };
	};

	function ModalNesting() {
		const [ isOpen, setOpen ] = useState( false );
		const [ isInnerOpen, setIsInnerOpen ] = useState( false );
		const [ { value: bodyOpenClassName }, cycleBodyOpenClassName ] = useReducer(
			cyclicReducer,
			[ 'open--first', 'open--second', 'open--third' ],
			( list ) => cyclicReducer( { index: -1 }, list )
		);
		return (
			el( Fragment, null,
				el( Button, { variant: "primary", onClick: () => setOpen( true ) }, "Open Modal" ),
				isOpen && (
					el( Modal, {
						onRequestClose: () => {
							setOpen( false );
						},
						bodyOpenClassName,
						style: { width: '30em' },
						title: 'Modal vs. body class names',
					},
						el( 'p', null, 'This modal has a custom ', el('code', null, 'bodyOpenClassName'), `.` ),

						isInnerOpen && el(
							Modal,
							{ onRequestClose: () => setIsInnerOpen( false ), title: `🪧 Hi`, style: { width: '30em' } },
							el( 'p', null, 'Currrent ', el('code', null, 'bodyOpenClassName'), ` is “${ bodyOpenClassName }” and may be changed with the following button.` ),
							el( Button, { variant: "secondary", onClick: () => cycleBodyOpenClassName() }, 'Change ', el('code', null, 'bodyOpenClassName') )
						),

						el( Button, { variant: "secondary", onClick: () => setIsInnerOpen( true ) }, 'Open Nested'),
					)
				)
			)
		);
	}
} )( wp );
  1. Copy the above snippet.
  2. Open a post or page.
  3. Paste and execute the copied snippet in the dev tools console.
  4. Press the "Open Modal" button added to the post settings ("Editor Settings" > "Modal demo").
  5. Note the body element class now includes "open--first".
  6. Open either the command palette or the keyboard shortcuts modals by way of their keyboard shortcuts.
  7. Note the body element class did not update to include "modal-open".
  8. Close the modal.
  9. Note the body element class still includes "open--first".

Automatically

  1. Checkout the first commit on this branch.
  2. Run modal tests npm run test:unit -- components/src/modal.
  3. Verify the two new tests fail.
  4. Switch back to the full branch.
  5. Repeat step 2.
  6. Verify the two new tests pass.

Screenshots or screencast

This demonstrates the two scenarios the added unit tests cover whereby the body class name is not currently added/removed as expected. The latter scenario is almost certainly never going to be encountered in the wild.

modal-body-class-fails.mp4

@stokesman stokesman added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Oct 17, 2023
Comment on lines -46 to +48
const level0Dismissers: MutableRefObject<
ModalProps[ 'onRequestClose' ] | undefined
>[] = [];
const ModalContext = createContext( level0Dismissers );
const ModalContext = createContext<
MutableRefObject< ModalProps[ 'onRequestClose' ] | undefined >[]
>( [] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A level0Dismissers reference isn't needed anymore. It was added in #51602 only to maintain existing behavior. Now the effect for adding/removing body classes is nesting agnostic and simplified in that aspect.

@stokesman stokesman marked this pull request as ready for review October 18, 2023 02:09
@mirka mirka requested a review from a team December 15, 2023 12:18
Copy link

Flaky tests detected in 29c962fa1bb6c34cae6bc6c06c18768f79eb9b43.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7263487451
📝 Reported issues:

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Makes sense, tests well!

Good to go with a changelog 👍

@stokesman
Copy link
Contributor Author

Thank you for reviewing Lena!

@stokesman stokesman merged commit fda1efc into trunk Dec 20, 2023
52 checks passed
@stokesman stokesman deleted the update/modal-body-classing branch December 20, 2023 06:28
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 20, 2023
artemiomorales pushed a commit that referenced this pull request Jan 4, 2024
* Add unit tests for body class name effects

* Fix and enhance body class attribute effect

* Add changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants