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: assess all position to placement conversions, add unit tests #44339

Closed
Tracked by #42770
ciampo opened this issue Sep 21, 2022 · 3 comments · Fixed by #44377
Closed
Tracked by #42770

Popover: assess all position to placement conversions, add unit tests #44339

ciampo opened this issue Sep 21, 2022 · 3 comments · Fixed by #44377
Assignees
Labels
[Package] Components /packages/components

Comments

@ciampo
Copy link
Contributor

ciampo commented Sep 21, 2022

With the refactor of Popover to using floating-ui internally (#40740), a new placement prop was introduced with the objective of replacing the legacy position prop.

Currently, there is a function converting position to placement, but:

  • its logic is complicated and difficult to follow
  • its logic could potentially miss some edge cases:
    • it looks like position accepted the undocumented format [yAxis] [xAxis] [corner])
    • it looks like the new placement prop doesn't allow the Popover to be placed centered on top of its anchor
  • its logic is not covered by unit tests (which makes the previous checks necessary)

Therefore, to improve the situation and get ready to deprecate the position prop, we should:

  1. Assess that all possible values of position are converted correctly (or in a best-effort way) to the corresponding placement
  2. Write unit tests
  3. Potentially make changes to the conversion logic to respect the tests added from the previous point
@ciampo ciampo changed the title Verify and potentially improve position to placement conversions, add unit tests Popover: assess all position to placement conversions, add unit tests Sep 21, 2022
@ciampo ciampo self-assigned this Sep 21, 2022
@ciampo ciampo added the [Package] Components /packages/components label Sep 21, 2022
@ciampo
Copy link
Contributor Author

ciampo commented Sep 21, 2022

I went ahead and checked out the repository before #40740 was merged. I was therefore able to render a Popover for all possible values of the position prop (even the ones that don't really make much sense), simulating a Popover wider and taller than its anchor.

I then repeated the same thing on trunk, taking a screenshot of all possible placement values.

Here's the comparison:

All position values

Popover wider than anchor Anchor wider than Popover
localhost_50240_iframe html_id=components-popover--all-placements args= viewMode=story (4) localhost_50240_iframe html_id=components-popover--all-placements args= viewMode=story (5)

All placement values

Popover wider than anchor Anchor wider than Popover
localhost_50240_iframe html_id=components-popover--all-placements viewMode=story localhost_50240_iframe html_id=components-popover--all-placements viewMode=story (2)

@ciampo
Copy link
Contributor Author

ciampo commented Sep 21, 2022

Based on the images above, this should be the expected conversion:

(disclaimer: for completeness, I listed all possible combinations of values for the position prop, even if some of them don't really make sense)

Position Placement Notes
middle top There is no equivalent placement in floating-ui, use top as fallback
bottom bottom -
top top -
middle left left -
middle center top There is no equivalent placement in floating-ui, use top as fallback
middle right right -
bottom left bottom-end Not a perfect match, but likely the closest
bottom center bottom -
bottom right bottom-start Not a perfect match, but likely the closest
top left top-end Not a perfect match, but likely the closest
top center top -
top right top-start Not a perfect match, but likely the closest
middle left left left -
middle left right left Not a perfect match, but likely the closest (not sure if this position even makes sense)
middle left bottom left-end Not a perfect match, but likely the closest (this one is really up to interpretation)
middle left top left-start Not a perfect match, but likely the closest (this one is really up to interpretation)
middle center left top There is no equivalent placement in floating-ui, use top as fallback (not sure if this position even makes sense)
middle center right top There is no equivalent placement in floating-ui, use top as fallback (not sure if this position even makes sense)
middle center bottom top There is no equivalent placement in floating-ui, use top as fallback (not sure if this position even makes sense)
middle center top top There is no equivalent placement in floating-ui, use top as fallback (not sure if this position even makes sense)
middle right left right Not a perfect match, but likely the closest (not sure if this position even makes sense)
middle right right right -
middle right bottom right-end Not a perfect match, but likely the closest
middle right top right-start Not a perfect match, but likely the closest
bottom left left bottom-end Not a perfect match, but likely the closest
bottom left right bottom-end -
bottom left bottom bottom-end Not a perfect match, but likely the closest (not sure if this position even makes sense)
bottom left top bottom-end Not a perfect match, but likely the closest (not sure if this position even makes sense)
bottom center left bottom not sure if this position even makes sense
bottom center right bottom not sure if this position even makes sense
bottom center bottom bottom not sure if this position even makes sense
bottom center top bottom not sure if this position even makes sense
bottom right left bottom-start -
bottom right right bottom-start Not a perfect match, but likely the closest
bottom right bottom bottom-start Not a perfect match, but likely the closest (not sure if this position even makes sense)
bottom right top bottom-start Not a perfect match, but likely the closest (not sure if this position even makes sense)
top left left top-end Not a perfect match, but likely the closest
top left right top-end -
top left bottom top-end Not a perfect match, but likely the closest (not sure if this position even makes sense)
top left top top-end Not a perfect match, but likely the closest (not sure if this position even makes sense)
top center left top not sure if this position even makes sense
top center right top not sure if this position even makes sense
top center bottom top not sure if this position even makes sense
top center top top not sure if this position even makes sense
top right left top-start -
top right right top-start Not a perfect match, but likely the closest
top right bottom top-start Not a perfect match, but likely the closest (not sure if this position even makes sense)
top right top top-start Not a perfect match, but likely the closest (not sure if this position even makes sense)

@ciampo
Copy link
Contributor Author

ciampo commented Sep 22, 2022

In #44377 I introduced a comprehensive set of unit tests to assess the current status of the returned placement values for every possible position.

*: bottom placements marked with an asterisks are fallback values used when position has middle center values, since there's not equivalent placement value

position placement expected by test specs placement received (as on trunk) matches expected value?
'middle' 'bottom'* undefined
'bottom' 'bottom' 'bottom'
'top' 'top' 'top'
'middle left' 'left' 'left'
'middle center' 'bottom'* 'center'
'middle right' 'right' 'right'
'bottom left' 'bottom-end' 'bottom-end'
'bottom center' 'bottom' 'bottom'
'bottom right' 'bottom-start' 'bottom-start'
'top left' 'top-end' 'top-end'
'top center' 'top' 'top'
'top right' 'top-start' 'top-start'
'middle left left' 'left' 'left'
'middle left right' 'left' 'left'
'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 left' 'right' 'right'
'middle right right' 'right' 'right'
'middle right bottom' 'right-end' 'right'
'middle right top' 'right-start' 'right'
'bottom left left' 'bottom-end' 'bottom-start'
'bottom left right' 'bottom-end' 'bottom-end'
'bottom left bottom' 'bottom-end' 'bottom-end'
'bottom left top' 'bottom-end' 'bottom-end'
'bottom center left' 'bottom' 'bottom-start'
'bottom center right' 'bottom' 'bottom-end'
'bottom center bottom' 'bottom' 'bottom'
'bottom center top' 'bottom' 'bottom'
'bottom right left' 'bottom-start' 'bottom-start'
'bottom right right' 'bottom-start' 'bottom-start'
'bottom right bottom' 'bottom-start' 'bottom-start'
'bottom right top' 'bottom-start' 'bottom-start'
'top left left' 'top-end' 'top-start'
'top left right' 'top-end' 'top-end'
'top left bottom' 'top-end' 'top-end'
'top left top' 'top-end' 'top-end'
'top center left' 'top' 'top-start'
'top center right' 'top' 'top-end'
'top center bottom' 'top' 'top'
'top center top' 'top' 'top'
'top right left' 'top-start' 'top-start'
'top right right' 'top-start' 'top-start'
'top right bottom' 'top-start' 'top-start'
'top right top' 'top-start' 'top-start'

The inconsistencies can be split into four main groups:

  1. 'middle' and 'middle center' positions: these values are not handles correctly by the positionToPlacement function, since the converted placement is either undefined or center (which is not even a valid placement). Since floating-ui doesn't have a corresponding placement for the middle center positions, my suggestion is to use a fallback value of bottom (which is also floating-ui's default value).
  2. 'middle [left|right] [bottom|top]' positions: for these positions, positionToPlacement currently returns vertically centered [left|right] placement, while the screenshots above suggest that the resulting placement should be aligned to the top or the bottom (i.e. '-start' or an '-end' alignment).
  3. '[bottom|top] center [left|right]' positions: for these positions, positionToPlacement currently returns an aligned -[start|end] placement, while the screenshots above suggest that the resulting placement should be centered on the x axis (ie. only 'bottom' or 'top' values)
  4. '[bottom|top] left left' positions: for these positions, positionToPlacement currently returns a '-start' alignment, while the screenshots above suggest more of an '-end' alignment. An '-end' alignment would also be more consistent with how other positions are converted (e.g top left is converted to an -end alignment and the popover looks like it's positioned in the same way, top right right is converted to a -start alignment...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants