Popover: tidy up and improve shifting behavior #42531
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +105 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
ciampo
added
[Type] Bug
talldan
reviewed
Jul 20, 2022
talldan
reviewed
Jul 22, 2022
| return offsetMiddleware( ( { placement } ) => { | ||
| const computedMainAxis = getMainAxisFromPlacement( placement ); | ||
| return { | ||
| mainAxis: computedMainAxis === 'x' ? -top : -left, |
Contributor
There was a problem hiding this comment.
I think there are a couple of fixes to be made here. If the main axis is x (horizontal), the offset should be left (also horizontal).
I think the problem is that getMainAxisFromPlacement is returning the wrong value. From the docs, the main axis is the one on which the distance between the reference and floating elements would be measure, so for a top or bottom placement you'd measure along the y axis.
I also noticed in #42417 that for top and left placements, the offset needs to be subtracted on the main axis, and for right and bottom placements added.
When that's all addressed, the code will be very similar to #42417, so perhaps it's worth reviewing/merging that PR? I tested it pretty thoroughly on a range of placements.
Contributor
Author
There was a problem hiding this comment.
When that's all addressed, the code will be very similar to #42417, so perhaps it's worth reviewing/merging that PR? I tested it pretty thoroughly on a range of placements.
That is definitely true, although this PR also adds a more refined behavior to limitShift, and several other minor improvements.
Probably the best course of action is to review / merge #42417 and then rebase this PR and refine the limitShift approach ?
Contributor
There was a problem hiding this comment.
Sounds good to me, thanks 👍
ntsekouras
force-pushed
the
try/popover-normalize-behavior
branch
from
84cefba to
36c0d69
Compare
Contributor
|
I rebased to better check things as this PR affects some popovers placement. |
67 tasks
ciampo
force-pushed
the
try/popover-normalize-behavior
branch
from
36c0d69 to
1102825
Compare
Contributor
|
Is this one ready for review @ciampo? |
Contributor
Author
Not yet, but I plan on marking it as such by Wed EOD |
…the anchor s virtual reference
ciampo
force-pushed
the
try/popover-normalize-behavior
branch
from
1102825 to
355333b
Compare
This was referenced Aug 3, 2022
Contributor
Author
|
I actually decided to split the work done in this PR across a few smaller, more focused PRs:
I'm going to close this PR, we can continue the conversation in the smaller PRs. |
Contributor
Author
|
Opened #43186 with fixes to the animation origin |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
The main changes in this PR are:
frameOffsetmiddleware to use theoffsetmiddleware instead of manually adding upxandycoordinateslimitShift()floating UI's option, in order to allow the floating element to fully shift past its reference when scrolling down in the site editor__unstableForcePositionpropuseLayoutEffecthook in thePopovercomponentPopoverworksYou can refer to the individual commits for an easier review of the changes.
Why?
(partially) fixes the regression flagged in #41575
alternative approach to #42417 and #42521
How?
Testing Instructions
So far, I've only tested these changes with the block toolbar in the site editor and post editor.
We should definitely test more throughly and across all different popover usages, but first I'd first like to establish if this is a potentially good approach, before investing more time in it
Screenshots or screencast
Kapture.2022-07-19.at.14.20.06.mp4