Deprecate the original slot implementation and refactor usage #22027
Conversation
youknowriad
added
[Feature] UI Components
| icon={ chevronDown } | ||
| label={ __( 'More rich text controls' ) } | ||
| controls={ orderBy( | ||
| fills.map( ( [ { props } ] ) => props ), |
There was a problem hiding this comment.
Mapping through fills and using them as "controls" is not something I was expecting. I'm not sure how we can solve this here? Why can't we just use a render Slot in renderContent of a "Dropdown" component. Anyway, @ellatrix, maybe you can help solve this. (bubbles virtually don't support children as function and can't support it)
There was a problem hiding this comment.
Why can't we just use a render Slot in renderContent of a "Dropdown" component.
Could you elaborate?
I'm also not sure how to fix this here. Normally we use a controls array with control objects, but in the case of RichText, components are used. Not sure if we can change that now.
There was a problem hiding this comment.
Pushed a fix, let me know what you think.
There was a problem hiding this comment.
How are they now sorted alphabetically?
There was a problem hiding this comment.
I'm also not sure how the collapsed button can now know if any buttons are active. I guess this would be easier with a controls array.
Another thing I'm noticing is that the buttons have too much space in between them compared to master
There was a problem hiding this comment.
They are not sorted, I believe they are probably sorted at the "import" level.
The thing is, with Slot/Fill, we're not supposed to know the shape of the children... So I'd say we were doing it wrong previously or Slot/Fill, is not the right API here.
There was a problem hiding this comment.
Hm, I tried converting it to control object but that doesn't really work because we're stuck with the React component rendered in the format type edit functions. Maybe we can create a slot and fill for each button, which could be sorted by the title of the format type?
There was a problem hiding this comment.
It still wouldn't work perfectly though, because some format types could render multiple buttons. That's more of an edge case though.
|
Size Change: +515 B (0%) Total Size: 821 kB
ℹ️ View Unchanged
|
|
Can we land those which work without issues first so we can test them faster in master? |
There's only two that are not working properly. I can extract the remaining ones to a separate PR but I'll have to remove the "deprecated" call to avoid breaking the tests. |
|
Right, you’d have to do it. You can always fix those issues but it might require some heavy debugging 😃 |
| @@ -25,13 +29,20 @@ const BlockSettingsMenuControlsSlot = ( { fillProps } ) => { | |||
| ), | |||
| }; | |||
| }, [] ); | |||
| const slot = useSlot( 'BlockSettingsMenuControls' ); | |||
| const hasFills = Boolean( slot.fills && slot.fills.length ); | |||
There was a problem hiding this comment.
It’s used nearly everywhere, how about useSlot exposes hasFills by default?
youknowriad
requested review from
ajitbohra,
chrisvanpatten,
etoledom,
jorgefilipecosta and
SergioEstevao
as code owners
youknowriad
force-pushed
the
update/deprecate-old-slot
branch
from
281e713 to
8b81838
Compare
|
Thanks for putting this together @youknowriad! :) I wonder what are the plans for React Native, since it doesn't support |
gziolo
added
the
Mobile App - i.e. Android or iOS
|
I think they're simply not using
|
|
Yes, it might be simply luck, but we aren't using any components that uses a Slot with the |
I was thinking the same, I feel we should have a unique implementation for slot/fill for web and a unique one for mobile. the current situation is not great. |
|
@youknowriad, do you plan to refresh this PR. Do you need any help from the team working on mobile apps? |
gziolo
added
the
[Status] Stale
|
Definitely, I'd love help from the mobile team here to ensure the APP works with all kind of slots and then I would be able to refactor this and move it forward. |
The original implementation of Slot (without bubblesVirtually) suffers from a number of issues, Important one being the lack of React context support. We've been slowing moving out from its usage but we forget sometimes to use
bubblesVirtuallywhen we add a new Slot.This PR deprecates the old implementation officially (deprecation message) and updates the existing usage.