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

Fix: Popover positions on the widget screen #15949

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 31, 2019

Description

This PR adds the Popover slot to the widget screen inside BlockEditorProvider as we do in the playground.
This allows the Popovers to appear in the correct position.

I'm not very convinced of this change. We are adding a Popover slot per BlockEditorProvider is it ok to do that and have the same slot multiple times on a page? I guess we will need something similar for slots like the block toolbar.

I tried another approach besides this one: Adding the PopOver slot in the layout of the widget screen as we do for edit-post. That did not work as the slot was not found. I think that the slot was not found because is only used inside the block editor. So the Slot usage is outside of SlotFillProvider descendants. In edit post that also seems to be the case, so why things work as expected there?

I also tried removing SlotFillProvider from the block editor and using it in the widget screen layout level with PopOver.slot as a direct child. That also fixed the popover position problem, but when I selected a paragraph from another widget area the paragraph that was previously selected gets double formatting controls. To test this we need to cherry pick #15948.

How has this been tested?

I went to the widget screen.
I added a paragraph, I opened block settings, I verified the popover with the menu appears on the right position.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels May 31, 2019
@youknowriad
Copy link
Contributor

Ok, I think I found the reason for this breakage when you try to use this in the layout :)
For Slot/Fill to work properly, we need to wrap everything in SlotFillProvider which means both the Popover.Slot and Popover should be rendered inside that provider. Right now, the SlotFillProvider is rendered inside the BlockEditorProvider which means you're forced to render the Popover.Slot inside the BlockEditorProvider.

I think there's a conceptual issue somewhere here. It looks like BlockEditorProvider shouldn't render SlotFillProvider and leave it for the "consumers": edit-post / edit-widgets and playground.

Alternatively, we could decide to have a Slot.Provider rendered inside BlockEditor.Provider but I think the theory behind Slot.Provider is that it should be only rendered once inside an application. And AFAIK not rendering it should also cause the Propover implementation to fallback to an implementation that works.

This also confirms my assumptions that having a global SlotFillProvider is probably not a good idea and I wonder whether we should revive #14408

I wonder also if there's a way to remove the need of SlotFillProvider as "context" has a default value, I wonder if we could leverage that default value to make sure that Slot/Fills could work globally as well?

For now, the actionable items could be:

1- Maybe we should deprecate the fallback or fix it.
2- We should probably remove SlotFillProvider from BlockEditorProvider and use it at the layout level.

cc @aduth as you know more about Slot/Fill

@aduth
Copy link
Member

aduth commented Jun 3, 2019

I think there's a conceptual issue somewhere here. It looks like BlockEditorProvider shouldn't render SlotFillProvider and leave it for the "consumers": edit-post / edit-widgets and playground.

For what it's worth, this is proposed in the draft pull request #14715, and was a necessary change for the work there in rendering Slots across distinct block editors:

https://github.com/WordPress/gutenberg/pull/14715/files#diff-f8ab9759c8d63240841c9d18565a9402

I was planning to refresh and extract some of this pull request into separate smaller tasks, in case this could be one of them.

@jorgefilipecosta
Copy link
Member Author

I was planning to refresh and extract some of this pull request into separate smaller tasks, in case this could be one of them.

Thank you @aduth, that would be very helpful. I guess the best action item now is to just land that extraction.

@aduth
Copy link
Member

aduth commented Jun 4, 2019

I was planning to refresh and extract some of this pull request into separate smaller tasks, in case this could be one of them.

See #15988

@jorgefilipecosta jorgefilipecosta force-pushed the fix/popover-position-widget-screen branch from 6d6e8c7 to bf38a0d Compare June 17, 2019 19:24
@jorgefilipecosta jorgefilipecosta removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jun 17, 2019
@jorgefilipecosta
Copy link
Member Author

This PR was updated taking advantage of #15988. Now it is a very minor change and should be ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants