feat(FloatingUI): Migrate from popperjs to FloatingUI - #4028
Conversation
✅ Deploy Preview for storybook-navy-digital-mod-uk ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
5c5a4ce to
a9ee5dd
Compare
|
thyhjwb6
left a comment
thyhjwb6
left a comment
There was a problem hiding this comment.
Code looks good, just a few comments.. maybe we need to chat about what is technically a breaking change.
useFloatingElement is used by useContextMenu which is used by the ContextMenu and that looks like this:
Maybe we just need to make sure FloatingBox is working as expected.
| export const useFloatingElement = ( | ||
| placement: Placement = 'bottom', | ||
| strategy: PositioningStrategy = 'fixed', | ||
| placement: Placement | 'auto' = 'bottom', |
There was a problem hiding this comment.
Technically this is a breaking change because we export useFloatingElement at the root of the component library. We don't actually document the use of useFloatingElement but we don't document other hooks either and I know they are being used in downstream apps. I wonder if we need to still reference the old types and do some mapping, so the types for the function parameters can be changed in v5 of the Design System.
| const isAutoPlacement = placement === 'auto' | ||
| const actualPlacement: Placement = isAutoPlacement ? 'bottom' : placement | ||
|
|
||
| // Build middleware array |
There was a problem hiding this comment.
Maybe we can remove this set of comments as they are also commented below.
|
|
||
| const arrowData = middlewareData.arrow | ||
|
|
||
| // Calculate arrow position based on Floating UI's coordinate system |
There was a problem hiding this comment.
I wonder if we could abstract a calculateArrowPosition function which would make this more self-documenting and negate the need for the comment.
| // depending on available space. Just verify it has positioning applied. | ||
| const floatingBox = screen.getByTestId('floating-box') | ||
| const style = floatingBox.style.transform | ||
| expect(style).toContain('translate') |
There was a problem hiding this comment.
Can we be more specific with the assertion? Isn't the test positioning always the same?
| expect(screen.getByTestId('floating-box')).toHaveStyle({ | ||
| transform: `translate(0px, ${offsetHeight}px)`, | ||
| }) | ||
| // Floating UI with flip middleware may position the box above or below |
There was a problem hiding this comment.
See comment about assertion.
2 participants
Overview
Migrate from popperjs to FloatingUI
Reason
FloatingUI is the replacement for popperjs and it provides some useful util hooks that can be used else where in the design-system (initially the Popover)
Work carried out
Developer notes
Seems like the functionality is the same between the components, maybe a few things are a pixel or two difference