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 position should be calculated based on is-from-[origin] #27993

Closed
jasmussen opened this issue Jan 5, 2021 · 3 comments
Closed

Popover position should be calculated based on is-from-[origin] #27993

jasmussen opened this issue Jan 5, 2021 · 3 comments
Labels
[Type] Bug An existing feature does not function as intended

Comments

@jasmussen
Copy link
Contributor

Popover menus are currently always centered according to the width of the opener element.

At the same time, for animation purposes, a 'is-from-' + origin can be applied. For most menus in block toolbar, that happens to be is-from-left.

Currently it looks right in most cases:

with compensation

But this is smoke and mirrors! The following CSS rule compensates for it:

Screenshot 2021-01-05 at 12 27 36

Those 25px happens to be the icon size of 24 plus the borderwidth of 1. Without this compensation margin, you can see what's up:

without compensation

The problem with this hack is that it falls apart for anything that isn't a 24px icon button:

Screenshot 2021-01-05 at 12 20 58

This is further exacerbated for buttons that are wider than a "Replace" button:

Screenshot 2021-01-05 at 12 21 13

In other words, the popover centering math could use a little enhancement:

  1. If the popover opens is-from-center, the current behavior is correct. See document outline:

Screenshot 2021-01-05 at 12 32 56

  1. If the popover opens from is-from-left, the popover needs to calculate its position based on the top left button of the opener. So, instead of this:

Screenshot 2021-01-05 at 12 39 56

It should be this:

Screenshot 2021-01-05 at 12 40 12

@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Jan 5, 2021
@jasmussen
Copy link
Contributor Author

CC: @ellatrix — do you have any insights on how hard this might be to address? There's some related code in https://github.com/WordPress/gutenberg/blob/master/packages/components/src/animate/index.js#L40 that the popover positioning should probably be aware of.

@tellthemachines
Copy link
Contributor

Related: #21275 where there has been a bit of back and forth about this already.

@jasmussen
Copy link
Contributor Author

Oh wow, thank you.

Closing this one in favor of #21275.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants