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: rework props around shift, filp and resize behavior #43191

Closed
Tracked by #42770
ciampo opened this issue Aug 12, 2022 · 5 comments · Fixed by #43546 or #43845
Closed
Tracked by #42770

Popover: rework props around shift, filp and resize behavior #43191

ciampo opened this issue Aug 12, 2022 · 5 comments · Fixed by #43546 or #43845
Assignees
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@ciampo
Copy link
Contributor

ciampo commented Aug 12, 2022

What

Tracked in #42770

Currently, Popover exposes two props for tweaking its behavior when intersecting with the viewport's boundaries:

  • __unstableForcePosition
  • __unstableShift

However, it feels like these props don't offer the correct abstraction and actually limit the flexibility of the component.

Proposed next steps

We could look at removing __unstableForcePosition and replacing it with separate resize and flip props, in order to be more aligned with floating-ui's middleware options.

Bonus: we should also look at https://ariakit.org/examples/popover, and try to make a potential refactor of the component to ariakit as simple as possible

@ciampo ciampo changed the title We should understand what's the best API strategy for defining the Popover's behaviour when reaching the viewport boundaries — currently, the Popover component exposes the __unstableForcePosition and __unstableShift props. We should come up with a definitive API strategy (should we allow turning on/off flip and shift separately?) and remove those __unstable props Popover: rework props around shift, filp and resize behavior Aug 12, 2022
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Aug 12, 2022
@talldan
Copy link
Contributor

talldan commented Aug 17, 2022

It turns out I didn't need this to fix the issue with legacy widgets (#43297).

But I can still push a PR that refactors this.

@talldan
Copy link
Contributor

talldan commented Aug 17, 2022

Bonus: we should also look at https://ariakit.org/examples/popover, and try to make a potential refactor of the component to ariakit as simple as possible

Nearly forgot, I looked at how ariakit's popover works.

It seems like that uses a fitViewport prop, which I guess is roughly the same as resize (though I don't know if 'viewport' is correct, but maybe that's just a nitpick):

And there's a flip prop:

So that may be another reason to still change the props.

@ciampo
Copy link
Contributor Author

ciampo commented Aug 17, 2022

Thank you for the exploration!

But I can still push a PR that refactors this.

That would be great!

@ciampo
Copy link
Contributor Author

ciampo commented Aug 30, 2022

Reopening as the shift prop is still marked as __unstable — we should:

  • add a new shift prop with the same default value of __unstableShift
  • add a deprecation notice to __unstableShift

@talldan , would you be able to work on this? It should be a quick one.

@ciampo
Copy link
Contributor Author

ciampo commented Sep 5, 2022

#43845

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
3 participants