WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.48k stars 4.18k forks source link

RFC: Improve Popover Positioning API #21275

Closed noahtallen closed 9 months ago

noahtallen commented 4 years ago

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

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: https://github.com/WordPress/gutenberg/issues/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:

Proposed API:

What if we changed the API to two values:

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

youknowriad commented 4 years ago

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.

gziolo commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

That sounds like a great idea :)

noahtallen commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 9 months ago

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