Refactor the Popover component to use the floatingUI library #40740
Conversation
youknowriad
requested review from
fullofcaffeine and
ntsekouras
and removed request for
a team
|
Size Change: -2.23 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
youknowriad
force-pushed
the
refactor/popover-floating-ui
branch
from
4d9c83e
to
3bcfe4b
Compare
|
I decided not to deprecate the position prop yet because we'd need to deprecate it on multiple components (button, dropdown, inserter...) so let's focus on the internals for now without API impact. |
|
I added a commit for this use case - hiding the popover if the anchored element is not in the view: Screen.Recording.2022-05-04.at.9.11.37.PM.mov |
There was a problem hiding this comment.
I haven't dived deep into the code on this PR yet, but I can see the proposal here is to use the floating-ui library instead of reakit (now renamed to ariakit and about to be released as stable).
I'm not familiar with floating-ui, but I wonder if we would be able to stick reakit under the hood — it would avoid introducing a new dependency, and it would be more coherent with the rest of the components library. Are there any particular issues with reakit popover (or with ariakit's ?)
|
@ciampo worth nothing reakit is working on using floating-ui as well. @diegohaz might confirm here. I think there's a lot of history to Popover in |
|
I also wanted to clarify that it's not about moving away from reakit since the current implementation doesn't use it, instead it uses custom computation algorithms which are removed here and replaced with floating-ui without any bundle size overhead (it's actually lower). |
That is correct — I should have specified that I referred to
Is that because using reakit's popover would require Popover to change its APIs in a breaking way (and become similar to how
It'd be great to hear from diego on this, so that we can align our plans a bit better in case. |
|
Ariakit is using Floating UI since ariakit/ariakit#1229. It's working great so far. That's the only external dependency we have. I don't think it's worth maintaining another positioning system like that with all the edge cases involved. So I agree that moving to Floating UI in Gutenberg is a good decision. Just make sure you don't expose any API from Floating UI. With that being said, the Ariakit Popover should be low-level enough (much more than in v1) if you want to migrate to it in the future. It even allows overwriting Floating UI altogether with your own positioning logic. It also addresses the issues I mentioned in this video (Focus management with React Portal). |
|
Potentially related: #41353 |
There was a problem hiding this comment.
Seems like the refactor here has caused some issues. It got me thinking: is it worth it to make this big change in one-go? Or maybe we should do this step-by-step like how we did with the Flyout component? We can get more eyes/reviews on the implementation, and we can do the migration more smoothly too.
FWIW, if we all agreed that the Popover's API is weird and hard to use, why don't we take this opportunity to rethink the best practices and learn from the mistakes? Instead of changing the internals of the existing weird component but still keep using it as-is.
| if ( ! refs.floating.current ) return; | ||
|
|
||
| Object.assign( refs.floating.current.firstChild.style, { | ||
| maxWidth: `${ width }px`, |
There was a problem hiding this comment.
I think this is causing the Site Editor's navigation button tooltip to be cut in the middle.
The expected width is supposed to be 115.63px but the applied maxWidth is 96px here.
There was a problem hiding this comment.
Thanks @kevin940726! I've got an open PR that looks to remove the maxWidth in #41361, while fixing a couple of other things (this was split off from a separate PR (#41268) that looked to fix up other positioning with the Tooltip).
There was a problem hiding this comment.
Oh, I want to clarify that I don't know if this was an issue already before this PR though!
|
I think the issues I saw so far are pretty minor for the gain brought by this PR.
I agree that it's better to do in multiple steps and that's one of the reasons I decided not to deprecated the position prop yet in this PR. For the internals change, I think it was necessary to do in one go, it's just not possible to change the implementation bit by bit to use floatingui. When it comes to Flyout, you can think of it as a Popover with a different API (not one that suits our usage of Popover) and a different implementation. This PR is actually a smaller step towards Flyout (keeping the current API which we need, and allowing us to iterate on it in a simpler way) |
|
What I mean was instead of patching the already complicated However, if the issues discovered so far are easy to fix then we sure can continue on this path forward. 👍 |
Oh got it, I think this strategy has been tried and discussed before but it falls short because we have to maintain the Popover component forever for third-party users so we'll end up with two components forever and because any migration will be painful for us and for our third-party users. In some cases it's a valid and the right strategy, especially when the API of a particular component is to be redone entirely, but in the case of our Popover component, we're not that far from the right API to suit our needs in the editor. There are two things that bother me in the component:
I guess this is me saying that we should definitely assess component per component but for this one, I'm on the camp of we should just continue using the existing one and improve it. |
What?
Right now, we have two popover components:
This PR consolidates by removing the second one (it's unused) and refactoring the first one to use floating-ui to drastically simplify it which allows us to be iterated on in terms of props... in order to make a better component without breaking changes.
There's some remaining work to do but the biggest/hardest part is done already.
Why?
Consolidation and a path forward for better APIs.
Notes
Testing Instructions
Test position and behavior of all kind of popovers: