gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

Popover: refine position-to-placement conversion logic, add tests

Open ciampo opened this issue 3 years ago • 0 comments

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 convertion 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 https://github.com/WordPress/gutenberg/issues/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?

  • I first introduced a comprehensive set of unit tests to check that every single possible value of position is converted to the expected placement value (following the expected specs in #44339)
  • I then replaced the custom logic in positionToPlacement with a simpler map of position-to-placement conversions

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 avatar Sep 22 '22 15:09 ciampo