Site Editor: make close button replaceable #22001

vindl · 2020-04-30T17:16:14Z

Description

The aim of this is to address the first part of #20929, namely making it easy to extend and replace the existing close button (W Icon) in site and post editor. Currently the code is contained to site editor only, but if it makes sense I will extend this approach to post editor too.

In order to allow for adding custom components the slot area was added in place of the existing close button. If no fills are provided it will render the default Close button, otherwise it will replace it with available fills.

For example, that could be achieved with the following code in the plugins:

registerPlugin( 'edit-site', {
    render() {
        return (
            <MainDashboardButton>
	        <CustomCloseButton />
	    </MainDashboardButton>
        );
    },
} );

How has this been tested?

Smoke testing with above POC code in site editor.
There should be no visual changes if the PR is used as is.
If you provide some custom fills, the default Close button should be replaced with you custom component.
Make sure that the case with multiple fills/plugins also works as expected.

Screenshots

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

vindl · 2020-04-30T17:16:57Z

packages/edit-site/src/components/header/close-area/index.js

+
+__experimentalSiteEditorCloseArea.Slot = Slot;
+
+export default __experimentalSiteEditorCloseArea;


I need to come up with a better name for this. :)

Yeah thats a tough one... should we replace Area with SlotFill ? or maybe there is a better term someone can suggest.

I don't think it's bad.

But, since this is the "home button", maybe __experimentalSiteEditorHomeButton?

Either one works IMO.

Yes, I'd avoid referring to "close" or any implicit action and try to be a bit more semantic (something like MainDashboardButton perhaps).

Renamed to MainDashboardButton in 555b399532aa0e2a02bbc07d24ce9c3f22d0d868.

github-actions · 2020-04-30T17:22:45Z

Size Change: +105 B (0%)

Total Size: 819 kB

Filename	Size	Change
`build/edit-site/index.js`	11.5 kB	+105 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.62 kB	0 B
`build/api-fetch/index.js`	4.08 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.6 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	761 B	0 B
`build/block-editor/index.js`	101 kB	0 B
`build/block-editor/style-rtl.css`	10.2 kB	0 B
`build/block-editor/style.css`	10.2 kB	0 B
`build/block-library/editor-rtl.css`	7.08 kB	0 B
`build/block-library/editor.css`	7.08 kB	0 B
`build/block-library/index.js`	115 kB	0 B
`build/block-library/style-rtl.css`	7.22 kB	0 B
`build/block-library/style.css`	7.23 kB	0 B
`build/block-library/theme-rtl.css`	683 B	0 B
`build/block-library/theme.css`	685 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	48.1 kB	0 B
`build/components/index.js`	179 kB	0 B
`build/components/style-rtl.css`	16.9 kB	0 B
`build/components/style.css`	16.9 kB	0 B
`build/compose/index.js`	6.66 kB	0 B
`build/core-data/index.js`	11.4 kB	0 B
`build/data-controls/index.js`	1.28 kB	0 B
`build/data/index.js`	8.44 kB	0 B
`build/date/index.js`	5.47 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	568 B	0 B
`build/dom/index.js`	3.1 kB	0 B
`build/edit-navigation/index.js`	4.05 kB	0 B
`build/edit-navigation/style-rtl.css`	485 B	0 B
`build/edit-navigation/style.css`	485 B	0 B
`build/edit-post/index.js`	28.1 kB	0 B
`build/edit-post/style-rtl.css`	12.2 kB	0 B
`build/edit-post/style.css`	12.2 kB	0 B
`build/edit-site/style-rtl.css`	5.18 kB	0 B
`build/edit-site/style.css`	5.18 kB	0 B
`build/edit-widgets/index.js`	7.77 kB	0 B
`build/edit-widgets/style-rtl.css`	4.67 kB	0 B
`build/edit-widgets/style.css`	4.66 kB	0 B
`build/editor/editor-styles-rtl.css`	428 B	0 B
`build/editor/editor-styles.css`	431 B	0 B
`build/editor/index.js`	44.3 kB	0 B
`build/editor/style-rtl.css`	5.07 kB	0 B
`build/editor/style.css`	5.08 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.64 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	712 B	0 B
`build/keyboard-shortcuts/index.js`	2.51 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.13 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	5.29 kB	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/primitives/index.js`	1.5 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.85 kB	0 B
`build/rich-text/index.js`	14.8 kB	0 B
`build/server-side-render/index.js`	2.67 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.02 kB	0 B
`build/viewport/index.js`	1.84 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.18 kB	0 B

_{compressed-size-action}

Addison-Stavlo

From a functionality standpoint this seems to work well!

The button continues to work as expected without any additions. Then by adding the filter/registration like you mentioned, I am able to override the button:

Yay slot fills! 🎉

epiqueras

I don't think we should register a new plugin and filter for this.

It would be more straightforward just to use a Slot that renders FullscreenModeClose if it doesn't have any fills.

I think @mtias and @youknowriad have more experience with this, so let's see what they think.

epiqueras · 2020-04-30T20:22:49Z

packages/edit-site/src/components/header/close-area/index.js

+
+__experimentalSiteEditorCloseArea.Slot = Slot;
+
+export default __experimentalSiteEditorCloseArea;


I don't think it's bad.

But, since this is the "home button", maybe __experimentalSiteEditorHomeButton?

Either one works IMO.

epiqueras · 2020-04-30T20:28:21Z

packages/edit-site/src/components/header/fullscreen-mode-close/index.js

@@ -21,6 +22,10 @@ function FullscreenModeClose() {
 		return null;
 	}

+	if ( applyFilters( 'siteEditor.closeButton.remove', false ) ) {


I think the convention is to use the full function name.

siteEditor.fullscreenModeClose.remove

I removed the filter in the latest refactor so this is no longer relevant.

epiqueras · 2020-04-30T20:30:46Z

packages/edit-site/src/index.js

@@ -13,6 +13,7 @@ import { render } from '@wordpress/element';
 */
 import './hooks';
 import './store';
+import './plugins';


I'm not sure it matters, but edit-post imports hooks, then plugins, then the store. We should probably mirror that just to be safe since these have side effects.

Also removed the plugins file in the refactor so this is no longer present.

vindl · 2020-04-30T23:09:46Z

It would be more straightforward just to use a Slot that renders FullscreenModeClose if it doesn't have any fills.

I was trying that option too but couldn't quite get to a satisfactory state with it, because FullscreenModeClose is also a fill, so the slot is never empty by default. Is there an existing example of that somewhere in the codebase?

Arguably the approach with optional removal is more versatile, because it also covers use-cases where someone might want to keep the button and add some other components next to it (not sure if this is desirable though?). Also in your case it's not clear what the expected behavior should be if multiple fills are provided by 3rd party plugins? Consistently with first operation, I'd expect only one fill will be shown, but that wouldn't be the case. And if we try to enforce it we end up needing something like priorities, which sounds very similar to a filters at that point. :)

epiqueras · 2020-05-01T00:49:44Z

I was trying that option too but couldn't quite get to a satisfactory state with it, because FullscreenModeClose is also a fill, so the slot is never empty by default. Is there an existing example of that somewhere in the codebase?

Use the render prop:

gutenberg/packages/edit-post/src/components/sidebar/post-status/index.js

Lines 27 to 52 in 4830c2c

    
           function PostStatus( { isOpened, onTogglePanel } ) { 
        
           	return ( 
        
           		<PanelBody 
        
           			className="edit-post-post-status" 
        
           			title={ __( 'Status & visibility' ) } 
        
           			opened={ isOpened } 
        
           			onToggle={ onTogglePanel } 
        
           		> 
        
           			<PluginPostStatusInfo.Slot> 
        
           				{ ( fills ) => ( 
        
           					<> 
        
           						<PostVisibility /> 
        
           						<PostSchedule /> 
        
           						<PostFormat /> 
        
           						<PostSticky /> 
        
           						<PostPendingStatus /> 
        
           						<PostSlug /> 
        
           						<PostAuthor /> 
        
           						{ fills } 
        
           						<PostTrash /> 
        
           					</> 
        
           				) } 
        
           			</PluginPostStatusInfo.Slot> 
        
           		</PanelBody> 
        
           	); 
        
           }

it also covers use-cases where someone might want to keep the button and add some other components next to it (not sure if this is desirable though?)

I don't even think the space is big enough for that, but they can always import the component and compose it themselves. Leaving it there is also assuming that whatever they "add" should come after and not before/between/etc. In rendering order.

Also in your case it's not clear what the expected behavior should be if multiple fills are provided by 3rd party plugins?

They all render, like usually.

Consistently with first operation, I'd expect only one fill will be shown, but that wouldn't be the case. And if we try to enforce it we end up needing something like priorities, which sounds very similar to a filters at that point. :)

That's not consistent. Having a "default" fill does not imply that only one fill can show at a time.

To cater to multiple plugins like that, you would need to filter the whole fill/button, but relying on that pattern and "priorities" doesn't scale well, and I think we try to avoid it where possible.

vindl · 2020-05-01T03:05:29Z

Use the render prop:

Thanks! I'll look into this next.

vindl · 2020-05-04T16:02:13Z

@epiqueras I removed the filter in the latest update, now it only relies on the presence of Fills. I also updated the PR summary and testing instructions to reflect this.

gziolo · 2020-05-04T16:12:53Z

You could also use withFilters HOC that wraps the default button which would work essentially the same with less code. @mtias do you have any preference? We have several filters that are used in places where we allow customizations like this (taxonomy, featured image, media upload, BlockEdit, etc). The approach with SlotFill is also interesting and I don’t remember if it was ever considered.

vindl · 2020-05-04T16:23:27Z

You could also use withFilters HOC that wraps the default button which would work essentially the same with less code. @mtias do you have any preference? We have several filters that are used in places where we allow customizations like this (taxonomy, featured image, media upload, BlockEdit, etc).

Thanks! We discussed the option of going with filters before and decided against it. From what I understood we are trying to limit their usage (@youknowriad might be able to provide more context here). From a developer experience standpoint, I think SlotFills are easier to use.

The approach with SlotFill is also interesting and I don’t remember if it was ever considered.

Based on my search through the codebase I don't think it has been used exactly in this form. Still, I think it is a cool pattern for replacing default components that we could use more in the future. It might even make sense to modify createSlotFill so that it can accept an optional parameter which would be the default component to render when no fills are provided.

mtias · 2020-05-04T18:45:56Z

Would we want to make MainDashboardButton.Slot unstable for a release or two? Also maybe we should expose an icon prop on it to make it easier to just swap it while retaining behaviour.

epiqueras · 2020-05-04T18:52:46Z

Would we want to make MainDashboardButton.Slot unstable for a release or two?

I don't think its warranted, but it doesn't hurt to be careful.

Also maybe we should expose an icon prop on it to make it easier to just swap it while retaining behaviour.

Great idea! @vindl do you want to follow up with that?

gziolo · 2020-05-04T18:55:43Z

Would we want to make MainDashboardButton.Slot unstable for a release or two?

Documentation is highly appreciated as well.

vindl · 2020-05-04T18:55:45Z

@epiqueras @mtias sure, I'll handle both of these items in a follow-up.

youknowriad · 2020-05-05T09:13:33Z

packages/edit-site/src/components/header/index.js

@@ -62,7 +62,7 @@ export default function Header( { openEntitiesSavedStates } ) {

 	return (
 		<div className="edit-site-header">
-			<FullscreenModeClose />
+			<MainDashboardButton.Slot />


see #22027

We're moving away from regular slots. Would you mind doing a small refactor here (bubblesVirtually slots don't support children render function

@youknowriad Actually I'm not sure how to achieve the same effect with this, given that it relies on children render function to handle the default case when no fills are provided?

There are examples of the same use-case in the code base. See here https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-inspector/index.js#L89-L90

Thanks, refactored this in #22179.

aduth · 2020-05-06T12:26:15Z

packages/edit-site/src/components/header/main-dashboard-button/index.js

+				return <FullscreenModeClose />;
+			}
+
+			return <> { fills } </>;


These spaces are significant, and cause React to create two additional text node children.

https://codepen.io/aduth/pen/ZEbrOYY

Suggested change

return <> { fills } </>;

return <>{ fills }</>;

Thanks for pointing it out! I removed this code in #22179.

vindl · 2020-05-07T16:14:30Z

Refactored this in #22179 according to the suggestions above.

vindl added the [Feature] Full Site Editing label Apr 30, 2020

vindl requested review from ockham, youknowriad, mtias, noahtallen, epiqueras and Addison-Stavlo April 30, 2020 17:16

vindl self-assigned this Apr 30, 2020

vindl commented Apr 30, 2020

View reviewed changes

Addison-Stavlo reviewed Apr 30, 2020

View reviewed changes

epiqueras reviewed Apr 30, 2020

View reviewed changes

mtias mentioned this pull request May 4, 2020

Iterations on W icon in the editor header #20929

Closed

2 tasks

vindl added 2 commits May 4, 2020 17:00

Site Editor: convert editor close button to slot

42c6e20

Remove filter and rely on presence of fills instead

75e6fe3

vindl force-pushed the update/close-button-extensible branch from 146717f to 555b399 Compare May 4, 2020 15:56

Rename to MainDashboardButton

566c91a

vindl force-pushed the update/close-button-extensible branch from 555b399 to 566c91a Compare May 4, 2020 16:10

epiqueras approved these changes May 4, 2020

View reviewed changes

vindl merged commit 1766e13 into master May 4, 2020

vindl deleted the update/close-button-extensible branch May 4, 2020 18:42

github-actions bot added this to the Gutenberg 8.1 milestone May 4, 2020

youknowriad reviewed May 5, 2020

View reviewed changes

gziolo added the Needs Dev Note Requires a developer note for a major WordPress release cycle label May 6, 2020

aduth reviewed May 6, 2020

View reviewed changes

vindl mentioned this pull request May 7, 2020

Site Editor: refactor close button slot #22179

Merged

6 tasks

vindl mentioned this pull request May 13, 2020

Close button: extensibility in post editor #22323

Merged

6 tasks

youknowriad mentioned this pull request May 14, 2020

Full Site Editing Breakdown #20791

Closed

53 tasks

ellatrix mentioned this pull request Jul 3, 2020

Dev Notes for WP 5.5 #23669

Closed

12 tasks

cguntur mentioned this pull request Oct 14, 2020

Dev Notes for WP 5.6 #26108

Closed

17 tasks

vindl removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Editor: make close button replaceable #22001

Site Editor: make close button replaceable #22001

vindl commented Apr 30, 2020 •

edited

Loading

vindl Apr 30, 2020

Addison-Stavlo Apr 30, 2020

epiqueras Apr 30, 2020

mtias May 1, 2020

vindl May 4, 2020

github-actions bot commented Apr 30, 2020 •

edited

Loading

Addison-Stavlo left a comment

epiqueras left a comment

epiqueras Apr 30, 2020

epiqueras Apr 30, 2020

vindl May 4, 2020

epiqueras Apr 30, 2020

vindl May 4, 2020

vindl commented Apr 30, 2020

epiqueras commented May 1, 2020

vindl commented May 1, 2020

vindl commented May 4, 2020

gziolo commented May 4, 2020

vindl commented May 4, 2020 •

edited

Loading

mtias commented May 4, 2020

epiqueras commented May 4, 2020

gziolo commented May 4, 2020

vindl commented May 4, 2020

youknowriad May 5, 2020

vindl May 7, 2020

youknowriad May 7, 2020

vindl May 7, 2020

aduth May 6, 2020

mtias May 6, 2020

vindl May 7, 2020

vindl commented May 7, 2020


		__experimentalSiteEditorCloseArea.Slot = Slot;

		export default __experimentalSiteEditorCloseArea;

Site Editor: make close button replaceable #22001

Site Editor: make close button replaceable #22001

Conversation

vindl commented Apr 30, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 30, 2020 • edited Loading

Addison-Stavlo left a comment

Choose a reason for hiding this comment

epiqueras left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindl commented Apr 30, 2020

epiqueras commented May 1, 2020

vindl commented May 1, 2020

vindl commented May 4, 2020

gziolo commented May 4, 2020

vindl commented May 4, 2020 • edited Loading

mtias commented May 4, 2020

epiqueras commented May 4, 2020

gziolo commented May 4, 2020

vindl commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vindl commented May 7, 2020

vindl commented Apr 30, 2020 •

edited

Loading

github-actions bot commented Apr 30, 2020 •

edited

Loading

vindl commented May 4, 2020 •

edited

Loading