-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
RFC: Improve Popover Positioning API #21275
Comments
The reasoning and the API make sense for me. I'll let @ellatrix chime in as she worked a lot on this component. Also, do you think we should rethinking how the smart behavior work? By smart behavior I mean three things:
And how these three smart behaviors can be combined together. |
You should consider using type Options = {|
placement: Placement, // "bottom"
modifiers: Array<$Shape<Modifier<any>>>, // []
strategy: PositioningStrategy, // "absolute",
onFirstUpdate?: ($Shape<State>) => void, // undefined
|};
type Placement =
| 'auto'
| 'auto-start'
| 'auto-end'
| 'top'
| 'top-start'
| 'top-end'
| 'bottom'
| 'bottom-start'
| 'bottom-end'
| 'right'
| 'right-start'
| 'right-end'
| 'left'
| 'left-start'
| 'left-end';
type Strategy = 'absolute' | 'fixed'; You can check details at https://popper.js.org/docs/v2/constructors/. |
Popper.js is great! Material UI uses it as well. From the smart features @youknowriad listed, the only thing I think Popper.js doesn’t support out of the box is automatically reducing the size when there’s no enough space. I guess it can be done with CSS though? |
It would be very nice to not have to implement all of this! So the suggestion would be to replace our core Popover component with this 3rd party one? Perhaps add a wrapper around it and export it from components? |
I was thinking about reimplementing the existing component using Popover from Reakit or Popper.js depending on how they fit to the existing component’s public API. We should keep it backward compatible. Many other components depend on it, including Tooltip. |
That sounds like a great idea :) |
I don't think we want to be limited by the existing API -- it would be nice to keep back compat while exposing a more robust API as well. |
If I understand correctly what you are describing, it should be achievable with the 3rd party modifier/plugin popper-max-size-modifier written by @atomiks |
Sharing also this article from the author of Popper.js that contains tips on positioning: https://dev.to/atomiks/everything-i-know-about-positioning-poppers-tooltips-popovers-dropdowns-in-uis-3nkl. |
Haiii~! Quickly dropping my thoughts here. 💯 for using a 3rd party solution like I agree with replacing the internals of However, I think it'll be worth it 💪 |
I think this can be closed as the Popover component has seen a lot of changes since this was created. |
The Popover positioning API is quite confusing. After spending a couple of hours with the code, I still don't exactly understand what everything does, and I assume my use case is impossible to accomplish in the existing format.
Current API
I don't understand this very well, so please correct my mistakes. I'm especially unsure what the x-axis and corner values do, but I think this highlights why the API should be improved. :)
Currently, the API is a single property
position
which is a space-separated string of three values. In sum, it looks like:bottom right
,middle center
(yikes), ortop left left
The main problem is that these are not composable in a flexible way. For example, if you specify
middle right left
, what should this accomplish? The y-anchor is located in the "middle", and it ought to open towards the "right", and use the "left" corner. As a result, the popup is anchored in the middle of the y-axis on the left edge of the box, and opens towards the right. This isn't easy to infer from the value passed.Additionally, this leaves a lot of popover possibilities impossible to accomplish. I've been looking at this issue: #20470. Essentially, I want a popover menu to be anchored to the top-right corner and open to the right and downwards. Based on intuition, one might think that
top right right
would work, but here is the result:After some trial and error, I was able to get close with
middle right right
:This puts the anchor in the y-axis middle of the right edge of the element. While it gives us something to work with, it doesn't position it quite how we want. One thing you may begin to notice is that there isn't a lot of flexibility for the direction in which we wish the popover to open.
Summary of issues:
Proposed API:
What if we changed the API to two values:
anchorLocation: { x: string, y: string }
popupDirection: { x: number, y: number }
anchorLocation
This would let us know where on the containing element we would like to anchor the popup. Here's a diagram:
For example,
anchorLocation: { x: 'bottom', y: 'right' }
indicates the bottom right corner of the element.popupDirection
This would control where the popup appears relative to the anchor location. My current thought is to allow each value to be one of
1, 0, -1
. Think of these values as being in a Cartesian coordinate system. Here's a diagram:Here are a some examples. To improve the clarity of the drawings, there is 1 unit of margin on each side of the "popup". In reality, the default margin would be flush with the anchor location on each side.
All together:
Here is an example with an anchor location and the direction specified.
Summary
These two props makes the API:
I think this sets us up for more flexibility as well. For example, the
anchorLocation
could be % values instead of strings. So I could specifyanchorLocation.x = 0.6
, and this would put the anchor location at 60% of the width of the element. This would provide extremely granular flexibility for the anchor location.Additionally, the
popupDirection
could allow granular numbers. So instead of specifyingx: 1
to indicate it should go right, we could consider those values to be inpx
, effectively allowing us to specify a distance in the x/y direction from the anchorPoint. (of course, this could probably just be accomplished with margin ad hoc)Please let me know if you have more, better ideas for this API. :) I think it could be a lot easier to use and a lot more powerful at the same time.
cc @youknowriad @ellatrix
The text was updated successfully, but these errors were encountered: