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

Popover: polish component after recent refactor #42770

Open
53 of 67 tasks
ciampo opened this issue Jul 28, 2022 · 8 comments
Open
53 of 67 tasks

Popover: polish component after recent refactor #42770

ciampo opened this issue Jul 28, 2022 · 8 comments
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement. [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@ciampo
Copy link
Contributor

ciampo commented Jul 28, 2022

The recent refactor of the Popover component to use floating ui has caused a few regressions, and still feels partially incomplete.

Known regressions (either issues, or PRs containing a fix):

Improvements wish list:

@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jul 28, 2022
@renatho
Copy link
Contributor

renatho commented Jul 28, 2022

Hey! I imagine it's probably related to what I'm facing with the Popover arrow.

Using WP 6.0 with Gutenberg plugin deactivated (working):

screen_shot_2022-07-28_at_11 13 12

With Gutenberg (trunk) plugin activated (not working):

screen_shot_2022-07-28_at_11 16 46

@ciampo
Copy link
Contributor Author

ciampo commented Jul 29, 2022

Hey! I imagine it's probably related to what I'm facing with the Popover arrow.

Hey @renatho , it could be related — could you open a separate issue for this regression (where you can add more details and we can discuss further), so that we keep this issue as a master tracking issue? Thank you!

@renatho
Copy link
Contributor

renatho commented Jul 29, 2022

Hey! I imagine it's probably related to what I'm facing with the Popover arrow.

Hey @renatho , it could be related — could you open a separate issue for this regression (where you can add more details and we can discuss further), so that we keep this issue as a master tracking issue? Thank you!

Sure! Here is the issue: #42820

@talldan
Copy link
Contributor

talldan commented Aug 2, 2022

I've added #42725 to the list. Hope that's ok!

@ciampo
Copy link
Contributor Author

ciampo commented Aug 2, 2022

I've added #42725 to the list. Hope that's ok!

Of course it is, thank you!

I must flag that I currently don't really have the capacity to work on this. I can try to squeeze some time and help, but we definitely need to understand who is going to work on these fixes

@ciampo ciampo added the [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. label Aug 4, 2022
@talldan
Copy link
Contributor

talldan commented Aug 12, 2022

The popover's offset doesn't update when the block/pattern inserter is closed (#42417 (comment)) (needs to be confirmed if the regression is caused by the refactor to floating UI)

I think I know what's happening here. There's no 'resize' listener for frameOffset, so the value isn't updated. It looks like it works ok when opening the sidebar, but this is actually the 'shift' middleware being triggered by the block's position changing.

The bug seems to specifically happen when inserting a new block and closing the sidebar, and I think that happens because the inserted block's toolbar is newly mounted, and a fresh frameOffset is calculated while the sidebar is open. Then, when closing the sidebar the frameOffset still retains the incorrect value.

I can work on a fix for this (edit: fix in #43172).

Interestingly, I also just noticed this in the floating-ui docs:
https://floating-ui.com/docs/react-dom#variables-inside-middleware-functions

It suggests that any variable accessed through a closure by middleware functions needs to be a ref. That would also apply to frameOffset.

@tellthemachines
Copy link
Contributor

👋 I just created #43541 for what seems to be a related issue; may be related to the recently merged #42887.

@talldan
Copy link
Contributor

talldan commented Aug 30, 2022

There are a couple of other issues here in the widgets customizer - Popover component overlay not fully visible in Customizer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement. [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
None yet
Development

No branches or pull requests

4 participants