Modal: add headerActions prop to render buttons in the header

[ramonjd]

What?

Not sure about this one yet. I would value some input 🙇🏻

This is an experiment to add a new prop headerActions: ReacNode to the <Modal /> component.

The prop allows folks to inject buttons and other elements into the header.

Why?

See: #53258 (comment)

Some folks working in the classic block require things to be roomier, and it'd be nice to be able to toggle fullscreen mode in the modal. Why? There might be a lot of complex content in there, for which it would be helpful to have more real estate to work.

How?

Adding a new prop.

TODO

• add stories

Testing Instructions

A good place to test this is by applying the new prop to the Class block modal:

Example diff

diff --git a/packages/block-library/src/freeform/editor.scss b/packages/block-library/src/freeform/editor.scss
index a3be7ffc5e..c09cf9aace 100644
--- a/packages/block-library/src/freeform/editor.scss
+++ b/packages/block-library/src/freeform/editor.scss
@@ -376,14 +376,16 @@ div[data-type="core/freeform"] {
 }
 
 .block-editor-freeform-modal {
-	.components-modal__frame {
+	.block-editor-freeform-modal__content {
 		// On large screens, make the TinyMCE edit area grow to take all the
 		// available height so that the Cancel/Save buttons are always into the
 		// view. On smaller screens, the modal content is scrollable.
 		@include break-large() {
 			// On medium and large screens, the modal component sets a max-height.
 			// We want the modal to be as tall as possible also when the content is short.
-			height: 9999rem;
+			&:not(.is-full-screen) {
+				height: 9999rem;
+			}
 
 			.components-modal__header + div {
 				height: 100%;
diff --git a/packages/block-library/src/freeform/modal.js b/packages/block-library/src/freeform/modal.js
index 9f7b20460c..61a6b89415 100644
--- a/packages/block-library/src/freeform/modal.js
+++ b/packages/block-library/src/freeform/modal.js
@@ -13,6 +13,29 @@ import {
 import { useEffect, useState, RawHTML } from '@wordpress/element';
 import { __ } from '@wordpress/i18n';
 import { useSelect } from '@wordpress/data';
+import { fullscreen } from '@wordpress/icons';
+import { useViewportMatch } from '@wordpress/compose';
+
+function ModalAuxiliaryActions( { onClick, isModalFullScreen } ) {
+	// 'small' to match the rules in editor.scss.
+	const isMobileViewport = useViewportMatch( 'small', '<' );
+	if ( isMobileViewport ) {
+		return null;
+	}
+
+	return (
+		<Button
+			onClick={ onClick }
+			icon={ fullscreen }
+			isPressed={ isModalFullScreen }
+			label={
+				isModalFullScreen
+					? __( 'Exit fullscreen' )
+					: __( 'Enter fullscreen' )
+			}
+		/>
+	);
+}
 
 function ClassicEdit( props ) {
 	const styles = useSelect(
@@ -58,6 +81,7 @@ export default function ModalEdit( props ) {
 		onReplace,
 	} = props;
 	const [ isOpen, setOpen ] = useState( false );
+	const [ isModalFullScreen, setIsModalFullScreen ] = useState( true );
 	const id = `editor-${ clientId }`;
 
 	const onClose = () => ( content ? setOpen( false ) : onReplace( [] ) );
@@ -78,6 +102,16 @@ export default function ModalEdit( props ) {
 					onRequestClose={ onClose }
 					shouldCloseOnClickOutside={ false }
 					overlayClassName="block-editor-freeform-modal"
+					isFullScreen={ isModalFullScreen }
+					className="block-editor-freeform-modal__content"
+					headerActions={
+						<ModalAuxiliaryActions
+							onClick={ () =>
+								setIsModalFullScreen( ! isModalFullScreen )
+							}
+							isModalFullScreen={ isModalFullScreen }
+						/>
+					}
 				>
 					<ClassicEdit id={ id } defaultValue={ content } />
 					<Flex

Also checkout the Story Book variation: npm run storybook:dev

Screenshots or screencast

2023-08-07.11.40.12.mp4

2023-08-09.11.09.01.mp4

✍️ Dev note

Thanks to a new headerActions prop, consumers of Modal can inject buttons (and other elements) into the Modal's header, next to the close button.

[t-hamano]

Thanks for the review, @ramonjd!

I've pulled the icons from the available ones in the library. They don't quite work. I think we need a minimize icon (or the opposite of resizeCornerNE)?

How about using only the fullscreen icon and toggling the isPressed prop?

Then, it might be better to have a gap of about 8px between the buttons:

Also, when in mobile view, the modal is always in full screen, so it is better to hide this button

Another approach is to make the Classic Editor modal full-screen from the start, instead of switching to full-screen status. When a user presses the Edit button in the Classic block, would they be surprised to see a large modal? 🤔 I would like to hear other opinions on this point.

[mirka]

I think we should tweak the architecture a bit. Are we expecting this "full screen mode" modal to have different styling, depending on the use case? I'm asking because in the example, the styling is added via the block-library package.

We either want the fullscreen styling built into the Modal component itself, or else support this feature in a more generic way. For example, we could enhance Modal so that a consumer can inject auxiliary buttons next to the Close button. Then, the Freeform block would inject a Fullscreen button, and do its own styling via the className prop.

What do you think?

[ramonjd]

Are we expecting this "full screen mode" modal to have different styling, depending on the use case? I'm asking because in the example, the styling is added via the block-library package.

I suppose that's a possibility. The freeform block does overwrite the modal styles (.components-modal__frame) at the moment so I expect it would be more sustainable if it didn't have to.

We either want the fullscreen styling built into the Modal component itself, or else support this feature in a more generic way. For example, we could enhance Modal so that a consumer can inject auxiliary buttons next to the Close button. Then, the Freeform block would inject a Fullscreen button, and do its own styling via the className prop.

Oh yeah, that's an interesting idea. Thanks! I wasn't too sure about this one when I pushed it so I'll play around. 🙇🏻

How about using only the fullscreen icon and toggling the isPressed prop?
Also, when in mobile view, the modal is always in full screen, so it is better to hide this button

Good ideas, thank you!

[ramonjd]

For example, we could enhance Modal so that a consumer can inject auxiliary buttons next to the Close button

I was thinking about this. What's the preferred way of defining these props? Do we need to be strict about what is passed?

I had started with a simple ReactNode for maximum flexibility, which would mean users could inject anything. Example:

<Modal
   auxiliaryActions={ <ButtonGroup /> }
/>

But I suppose there's also the option of reining it into an array of buttonProps objects each of which would render a <Button ... />.

<Modal
   auxiliaryActions={ [
       {
          ...someButtonProps,
       }
    ] }
/>

[ramonjd]

I had started with a simple ReactNode for maximum flexibility, which would mean users could inject anything

I've done this for now, just as a placeholder. Happy to implement further advice/ideas. Thank you @t-hamano and @mirka !!

[andrewserong]

I had started with a simple ReactNode for maximum flexibility, which would mean users could inject anything.

With Modal being fairly generic, perhaps allowing folks to inject anything might be preferable?

[ramonjd]

I've added a TODO to add a story once we've decided on an approach. 👍🏻

[andrewserong]

This is testing very nicely for me!

2023-08-08.16.24.57.mp4

In terms of naming, I don't feel too strongly about it, but I was wondering if secondaryActions might work? Mostly because auxiliary can be hard to spell 😄. On the other hand, since there's no primary or actions prop, maybe auxiliary is better.

Anyway, the injection and story all look good to me, looks like the changelog file might need a rebase, but once there's consensus on the naming, this one LGTM! ✨

[ramonjd]

I don't feel too strongly about it, but I was wondering if secondaryActions might work? Mostly because auxiliary can be hard to spell

Thanks @andrewserong

You know, I probably had to fix my own typos about 20 times when typing it out. 😆

I'm also agnostic here. 'complementary'?

[andrewserong]

I'm also agnostic here. 'complementary'?

Oh, that could work, too. And I only see one typo for complimentary in the repo 😁. The words auxiliary, secondary, and complementary appear to be used in at least one other place in the repo, so all sound like reasonable options to me. Let's see if @mirka or anyone else have any preferences 😄

[ramonjd]

And I only see one typo for complimentary in the repo 😁.

Nice find!

I couldn't help myself 😄

• Docs: complimentary > complementary #53413

[ramonjd]

headerActions was suggested as an alternative. I like it. I'll update it to that and apologize later 😄

[noisysocks]

I like actions or headerActions as a name because tertiary, secondary, auxiliary, etc. are a bit too abstract for my dumb brain.

Just flagging hat we have InspectorPopoverHeader which I added as a separate component because I wasn't sure back then whether we'd want something like this for all modals. How do you feel about deprecating/removing InspectorPopverHeader as part of this?

[ramonjd]

I like actions or headerActions as a name because tertiary, secondary, auxiliary, etc. are a bit too abstract for my dumb brain.

You got it!!! 👍🏻

Just flagging hat we have InspectorPopoverHeader which I added as a separate component because I wasn't sure back then whether we'd want something like this for all modals. How do you feel about deprecating/removing InspectorPopverHeader as part of this?

If you think we should, for sure.

I can do a follow up PR because it might involve extricating it from the PublishDateTimePicker

gutenberg/packages/block-editor/src/components/publish-date-time-picker/index.js

Line 12 in 63b1f40

import InspectorPopoverHeader from '../inspector-popover-header';

[noisysocks]

True. I guess those components need to contain the Popover. A little bit of refactoring required.

[andrewserong]

Looking good, and still tests nicely after the prop name switch 👍

LGTM! ✨

[ramonjd]

Thanks for re-testing @andrewserong 🙇🏻

I'll let this PR stew overnight in case there's further feedback and merge tomorrow.

[mirka]

Btw I just noticed that Storybook isn't showing TypeScript data in the props tables anymore 😬 Something must have broke at some point. We'll look into that.

[mirka]

I think this name is good too 👍 We should definitely have "header" in the name. For a moment I thought maybe "extras" instead of "actions" would be more accurately generic. But on the other hand I think it's good to restrict/suggest that it's intended for button-ish actions, just for the sake of UI design consistency.

[ramonjd]

True. I guess those components need to contain the Popover. A little bit of refactoring required.

Looking at this a bit more closely, InspectorPopverHeader is used in other contexts, e.g., the <Dropdown /> popover in the post template controls and page panel status. It still seems useful in those contexts and not sure there's a specific crossover with the <Modal /> component.

[noisysocks]

Oh right I've mixed Modal and Popover up. I still think it might make sense for Modal, Popover and Dropdown to all have a headerActions prop which works consistently.

[ciampo]

Added a dev note

[ramonjd]

Thanks @ciampo 🙇🏻