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

Update the Inspector slots to use the bubblesVirtually slots #16807

Merged
merged 1 commit into from Nov 29, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Jul 30, 2019

Continuation of #16421
Closes #9203

In several occasions, we noticed issues with the default Slot/Fill implementation (the one that copes children from the Fill into the Slot). These issues make it for example impossible to use A Slot inside a Fill. The React context doesn't propagate properly in the regular SlotFill implementation. The "bubblesVirtually" (Portal-based) implementation doesn't suffer from these issues.

#16421 updated the block toolbar to rely on bubblesVirtually, the current PR expands that work for the inspector slots.

Testing instructions

  • Check that the block inspector still works as expected
</PanelBody>
) }
</InspectorAdvancedControls.Slot>
<__experimentalSlotFillConsumer>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 30, 2019

Author Contributor

I'm not totally satisfied that I had to expose the consumer. The problem is, in the portal-based slot implementation, the portal should be rendered and can't support the same children as function prop we have in the other implementation (there's no "children" to be passed since it's all handled by React).

This comment has been minimized.

Copy link
@aduth

aduth Jul 30, 2019

Member

Hmm, is there a way we could assign the slot's children function as a property of the slot (useSlot) that can be run from the Fill ?

Or something like a getChildren function called by the fill?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 1, 2019

Author Contributor

But that's not what we want, the requirement here is that we don't want to show the panel component that is rendered as a parent of a slot, if there's no fill for that particular slot.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 1, 2019

Author Contributor

I kept thinking about this and I don't see a better approach for the moment.

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 9, 2019

Member

Have you considered exposing InspectorAdvancedControls.Consumer instead? It could have the following characteristics:

  • hasFills - boolean value
  • slot - would be a shortcut for InspectorAdvancedControls.Slot bubblesVirtually />

In code:

<InspectorAdvancedControls.Consumer>
    { ( { hasFills, slot } ) =>
        hasFills && (
            <PanelBody { ...panelProps }>
                { slot }
            </PanelBody>
        )
    }
</InspectorAdvancedControls.Consumer>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 9, 2019

Author Contributor

How do you build this InspectorAdvancedControls.Consumer ?

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 9, 2019

Member

I still don't know how to exactly solve it :)

@youknowriad youknowriad requested review from aduth and WordPress/gutenberg-core Jul 30, 2019
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Aug 9, 2019

In my testing, this PR works properly after the changes applied. Let's agree on the implementation and move forward.

@gziolo gziolo mentioned this pull request Nov 5, 2019
5 of 5 tasks complete
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Nov 27, 2019

@youknowriad Is this still something we want? Could you rebase the PR? Then I'll give it a test and review so we can merge it.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Nov 27, 2019

yes, I still want that personally, I think we should deprecate the non-bubble virtually slots entirely. I'll rebase

@youknowriad youknowriad force-pushed the update/inspector-bubbles-virtually branch from c56cd29 to eedc353 Nov 27, 2019
@@ -105,6 +107,10 @@ class SlotFillProvider extends Component {
return sortBy( this.fills[ name ], 'occurrence' );
}

hasFills( name ) {
return this.fills[ name ] && !! this.fills[ name ].length;

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 27, 2019

Member

In a lot of other places, we just check fills.length. Is the extra method needed?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 27, 2019

Author Contributor

Can't say for sure, this was a long time ago :P

Copy link
Member

ellatrix left a comment

Tested a RichText instance in the inspector, and it works really well!
Would maybe be good to have an e2e test for it?

@youknowriad youknowriad merged commit 6447a09 into master Nov 29, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Canceled
Details
@youknowriad youknowriad deleted the update/inspector-bubbles-virtually branch Nov 29, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.