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

RFC: Improve Popover Positioning API #21275

Closed
noahtallen opened this issue Mar 30, 2020 · 11 comments
Closed

RFC: Improve Popover Positioning API #21275

noahtallen opened this issue Mar 30, 2020 · 11 comments
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@noahtallen
Copy link
Member

noahtallen commented Mar 30, 2020

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), or top left left

  • y-axis: where the first portion of the string is the y positioning, which can be one of (top, middle, bottom).
  • x-direction: Not quite the x-axis. Rather, the direction in which the popover is meant to open.
  • corner: Currently undocumented, this seems to set the corner of the element (or the corner of the popover???) to which the popover should be anchored. Can be top, bottom, left, and right.

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:

Screen Shot 2020-03-30 at 3 40 53 PM

After some trial and error, I was able to get close with middle right right:

Screen Shot 2020-03-30 at 3 41 52 PM

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:

  • Hard to reason about
  • Difficult to know which values will give you the result you want.
  • Not very flexible in terms of the different positions available.
  • Even less flexible in terms of the directions in which the modal can open.

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:

Scanned Documents 1

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:

Scanned Documents 2

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.

Scanned Documents 3

All together:

Here is an example with an anchor location and the direction specified.

Scanned Documents 4

Summary

These two props makes the API:

  1. Very straightforward to consume.
  2. Extremely flexible to work with, while still being sensible.

I think this sets us up for more flexibility as well. For example, the anchorLocation could be % values instead of strings. So I could specify anchorLocation.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 specifying x: 1 to indicate it should go right, we could consider those values to be in px, 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

@noahtallen noahtallen added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Mar 30, 2020
@youknowriad
Copy link
Contributor

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:

  • Reducing the size of the popover when there's not enough space available in the window.
  • Automatically changing the anchor location and potentially the popup direction if there's not enough space on a given axis.
  • Shifting the popover to the left/right/top/bottom (instead of resizing) when the edge of the window is reached.

And how these three smart behaviors can be combined together.

@gziolo
Copy link
Member

gziolo commented Mar 31, 2020

You should consider using Popper.js library that is MIT licensed and weighs 3kB. Reakit uses it internally so @diegohaz can share more details if you would be interested. It has very advanced API, it supports the following options out of the box:

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/.

@diegohaz
Copy link
Member

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?

@noahtallen
Copy link
Member Author

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?

@gziolo
Copy link
Member

gziolo commented Mar 31, 2020

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.

@noahtallen
Copy link
Member Author

That sounds like a great idea :)

@noahtallen
Copy link
Member Author

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.

@FezVrasta
Copy link

FezVrasta commented Apr 3, 2020

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.

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

@gziolo
Copy link
Member

gziolo commented May 3, 2020

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.

@ItsJonQ
Copy link

ItsJonQ commented Jul 27, 2020

Haiii~! Quickly dropping my thoughts here.

💯 for using a 3rd party solution like Reakit/Popover. Mentioned above, it uses the popper.js positioning engine (which is a brilliant feat of engineering). I've had amazing experiences with both solutions previously.

I agree with replacing the internals of Popover with the 3rd party solution.
As others have mentioned, it will be a challenge (due to prior implementations, dependencies like Tooltip, etc...)

However, I think it'll be worth it 💪

@noahtallen
Copy link
Member Author

I think this can be closed as the Popover component has seen a lot of changes since this was created.

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.
6 participants