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.

Sign up for GitHub

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

Jump to bottom

Popover: Allow legitimate 0 positions to update popover position #51320

Merged
merged 5 commits into from
Jun 12, 2023
Merged

Popover: Allow legitimate 0 positions to update popover position #51320

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ea42d0
Popover: Allow legitmate 0 positions to update popover position
andrewserong Jun 8, 2023
f81f2ad
Add changelog entry
andrewserong Jun 8, 2023
86fb707
Refactor to utility function
ciampo Jun 8, 2023
50475b5
Add unit tests
ciampo Jun 8, 2023
463041f
Switch to undefined fallback for null and NaN cases, update snapshots
andrewserong Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fix

- `Popover`: Allow legitimate 0 positions to update popover position ([#51320](https://github.com/WordPress/gutenberg/pull/51320)).

## 25.1.0 (2023-06-07)

### Enhancements
Expand Down
10 changes: 8 additions & 2 deletions packages/components/src/popover/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,8 +499,14 @@ const UnforwardedPopover = (
// to use `translateX` and `translateY` because those values would
// be overridden by the return value of the
// `placementToMotionAnimationProps` function in `AnimatedWrapper`
x: Math.round( x ?? 0 ) || undefined,
y: Math.round( y ?? 0 ) || undefined,
x:
x === null || Number.isNaN( x )
? 0
Copy link
Contributor Author

@andrewserong andrewserong Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thoughts, perhaps this side of the ternary should be undefined as prior to this PR, the || meant that nullish or NaN values resulted in undefined... I can give that a try tomorrow, as that might potentially resolve the issue with the failing snapshot tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To sum up, here are the different versions for this algorithm tried so far:

  • this pr: c === null || Number.isNaN( c ) ? 0 : Math.round( c )
  • trunk (which introduced the regression): Math.round( c ?? 0 ) || undefined
  • old version: Number.isNaN( c ) ? 0 : c ?? undefined

I guess we need to understand exactly in which scenario we'd need to have undefined instead of 0. @corentin-gautier maybe can also help, since he worked on #46187 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for outlining each of the different options. To try to be more conservative about it, I've updated this logic in this PR to c === null || Number.isNaN( c ) ? undefined : Math.round( c ) so that we're only outputting a number when we have a real value. I think that might get us to a smaller potential change? Happy to revert if need be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good to me. Looking at the snapshot changes, it's interesting that motion stops applying a transform on the Z axis — it must be some internal framer motion optimisation. From past experience, usually the reason for having translateZ(0) would be to promote an element to a separate composite layer, thus reducing the amount of times it needs to be re-painted to screen. But I can see that we're already applying the will-change: transform style, which also achieves the same goal, so we should also be fine under that point of view.

: Math.round( x ),
y:
y === null || Number.isNaN( y )
? 0
: Math.round( y ),
}
}
>
Expand Down