Site Editor: refactor close button slot #22179
Conversation
vindl
requested review from
ajitbohra,
chrisvanpatten,
mkaz and
youknowriad
as code owners
vindl
requested review from
noahtallen,
ockham,
Addison-Stavlo,
mtias,
aduth and
gziolo
|
Size Change: +1.07 kB (0%) Total Size: 824 kB
ℹ️ View Unchanged
|
vindl
changed the title
| @@ -21,10 +20,12 @@ function FullscreenModeClose() { | |||
| return null; | |||
| } | |||
|
|
|||
| const buttonIcon = ! isEmpty( icon ) ? icon : wordpress; | |||
There was a problem hiding this comment.
| const buttonIcon = ! isEmpty( icon ) ? icon : wordpress; | |
| const buttonIcon = icon || wordpress; |
When would an icon ever be falsy?
| const CloseButton = () => { | ||
| const slot = useSlot( MainDashboardButton.slotName ); | ||
| const hasFills = Boolean( slot.fills && slot.fills.length ); | ||
|
|
||
| if ( ! hasFills ) { | ||
| return <FullscreenModeClose />; | ||
| } | ||
|
|
||
| return <MainDashboardButton.Slot bubblesVirtually />; | ||
| }; |
There was a problem hiding this comment.
This should be defined in the slot's file.
|
|
||
| ```js | ||
| import { registerPlugin } from '@wordpress/plugins'; | ||
| import { __experimentalMainDashboardButton } from '@wordpress/edit-site'; |
There was a problem hiding this comment.
so this slot is just an edit-site slot? Would we need to offer the same for the other screens. Also, if the fallback button is the same across screens, should the slot be the same across screens too?
There was a problem hiding this comment.
This shouldn't block this PR. I think it's ready.
There was a problem hiding this comment.
so this slot is just an edit-site slot?
Yes.
Would we need to offer the same for the other screens. Also, if the fallback button is the same across screens, should the slot be the same across screens too?
I was planning to make a separate slot for post editor. The rationale being that someone might want to provide a different component in other screens. Or it might look the same but we might want it to behave differently.
There was a problem hiding this comment.
I'm not 100% certain we need a separate component especially if we add support to more and more screens (widgets...). Also it seems possible to enqueue the extension based on the screen or check the screen and change the behavior.
There was a problem hiding this comment.
Also it seems possible to enqueue the extension based on the screen or check the screen and change the behavior.
That's true. Since I was planning on starting a separate PR for post editor, would it be fine if I consolidate this to use the same slot in it and merge this as is for now?
Or I could expand this PR to take care of both. 🤔I generally prefer to keep them smaller.
There was a problem hiding this comment.
Yes, we should merge this and see in a separate PR if we can have the same slot.
Description
This is a follow up to #22001 that addresses some of the outstanding feedback there. A couple of things were changed when compared to the previous version:
bubblesVirtuallyin order to make this compliant with Deprecate the original slot implementation and refactor usage #22027wordPressLogoicon with the one from the@wordpress/iconspackage. This was done in order to align it with latest changes in post editor.MainDashboardButtonFor the last item I experimented with a couple of things. I didn't see a way to make it work with fill props since we are replacing the whole component that we want to customize if the fill is present. I tried adding a separate slot just for the icon. While that works it seems unnecessarily convoluted. In the end I settled for exporting the existing close button component from the
edit-sitepackage so it can be customized via props and used as a fill.How has this been tested?
If you want to replace the whole component:
If you just want to customize the icon:
Screenshots
Types of changes
Checklist: