-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: refine position-to-placement conversion logic, add tests #44377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice @ciampo 👍
This is looking great. The changes certainly make it much easier to grok positionToPlacement
.
There were two minor typos that I think we can fix up but other than that. LGTM!
✅ No build or typing errors
✅ Popover unit tests pass
cef0130
to
3113b35
Compare
### Bug Fix | ||
|
||
- `Popover`: fix limitShift logic by adding iframe offset correctly [#42950](https://github.com/WordPress/gutenberg/pull/42950)). | ||
- `Popover`: refine position-to-placement conversion logic, add tests ([#44377](https://github.com/WordPress/gutenberg/pull/44377)). | ||
|
||
### Internal | ||
|
||
- `Mobile` updated to ignore `react/exhaustive-deps` eslint rule ([#44207](https://github.com/WordPress/gutenberg/pull/44207)). | ||
- `Popover`: refactor unit tests to TypeScript and modern RTL assertions ([#44373](https://github.com/WordPress/gutenberg/pull/44373)). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also moved a few changelog entries from recent PRs to the Unreleased
section, since they had been wrongly added under the 21.1.0
release
Closes #44339
Requires #44373 to be merged first
What?
This PR improves the logic to convert values for the
Popover
(legacy)position
prop, to the newly introducedplacement
prop. Until now, the conversion was done via thepositionToPlacement
utility function, which doesn't cover all possible cases and has a quite complex logic.The newly proposed logic is based on the investigation carried out in #44339, so please check out that issue to see the reasoning behind the code changes from this PR.
This is the first step of a plan that aims at:
position
usages toplacement
position
prop and scheduling it for deletionposition
propWhy?
Almost all consumers of
Popover
still used theposition
prop, and therefore we need to make sure that the conversion fromposition
toplacement
is implemented correctly.How?
position
is converted to the expectedplacement
value (following the expected specs in Popover: assess allposition
toplacement
conversions, add unit tests #44339)positionToPlacement
with a simpler map of position-to-placement conversionsTesting Instructions
Apart from code readability, the differences between the converted
placement
values in this PR vstrunk
are:position
placement
(this PR)placement
(trunk
)'middle'
'bottom'
*undefined
'middle center'
'bottom'
*'center'
'middle left bottom'
'left-end'
'left'
'middle left top'
'left-start'
'left'
'middle center left'
'bottom'
*'center'
'middle center right'
'bottom'
*'center'
'middle center bottom'
'bottom'
*'center'
'middle center top'
'bottom'
*'center'
'middle right bottom'
'right-end'
'right'
'middle right top'
'right-start'
'right'
'bottom left left'
'bottom-end'
'bottom-start'
'bottom center left'
'bottom'
'bottom-start'
'bottom center right'
'bottom'
'bottom-end'
'top left left'
'top-end'
'top-start'
'top center left'
'top'
'top-start'
'top center right'
'top'
'top-end'
*:
bottom
placements marked with an asterisks are fallback values used whenposition
hasmiddle center
values, since there's not equivalentplacement
valueNone of the positions listed in the table above is currently used in Gutenberg, and therefore:
Popover
component in a very significant way.