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: refine position-to-placement conversion logic, add tests #44377

Merged
merged 5 commits into from
Sep 23, 2022

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 22, 2022

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 introduced placement prop. Until now, the conversion was done via the positionToPlacement 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:

  • converting all position usages to placement
  • deprecating the position prop and scheduling it for deletion
  • deleting the position prop

Why?

Almost all consumers of Popover still used the position prop, and therefore we need to make sure that the conversion from position to placement is implemented correctly.

How?

Testing Instructions

Apart from code readability, the differences between the converted placement values in this PR vs trunk are:

position new placement (this PR) old 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 when position has middle center values, since there's not equivalent placement value

None of the positions listed in the table above is currently used in Gutenberg, and therefore:

  • there isn't really a way to test these changes, apart from creating an ad-hoc Storybook example and check it before and after the changes from this PR
  • at the same time, this means that this PR is basically guaranteed not to introduce any regressions in the Gutenberg app
  • and more in general, the changes that this PR introduces are quite marginal. Therefore, even a potential regression introduced by this PR wouldn't affect the usage of the Popover component in a very significant way.

@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Sep 22, 2022
@ciampo ciampo self-assigned this Sep 22, 2022
@ciampo ciampo marked this pull request as ready for review September 22, 2022 17:14
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a 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

packages/components/src/popover/utils.ts Outdated Show resolved Hide resolved
packages/components/src/popover/utils.ts Outdated Show resolved Hide resolved
Base automatically changed from feat/popover-test-typescript to trunk September 23, 2022 07:10
@ciampo ciampo force-pushed the feat/popover-position-to-placement-refinements branch from cef0130 to 3113b35 Compare September 23, 2022 07:20
Comment on lines +5 to +14
### 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)).

Copy link
Contributor Author

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

@ciampo ciampo merged commit 0a9402a into trunk Sep 23, 2022
@ciampo ciampo deleted the feat/popover-position-to-placement-refinements branch September 23, 2022 08:15
@github-actions github-actions bot added this to the Gutenberg 14.3 milestone Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popover: assess all position to placement conversions, add unit tests
2 participants