Skip to content
This repository has been archived by the owner on Apr 14, 2024. It is now read-only.

GOOD-94: migrate popperjs to floating-ui #11

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

shaswat-indian
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, api changes) - change in the `usePosition` hook signature
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Same as the new behavior - tested with existing stories.

Issue Number: N/A

What is the new behavior?

Screen.Recording.2024-01-17.at.10.39.42.PM.mov

Does this PR introduce a breaking change?

[x] Yes
[ ] No

This PR migrate the dependency from PopperJS to floating-UI. This may be a breaking change for certain use cases where the usePosition hook is used, however this should not impact the use cases which are included in the storybook as the behavior looks identical even after the change.

Other information

@shaswat-indian shaswat-indian requested a review from amcdnl January 17, 2024 17:23
@ghsteff
Copy link
Contributor

ghsteff commented Jan 17, 2024

Just a heads up, I tested this locally with reablocks and Tooltip seems to be working with different positionings straight outta the box 🎉

But when testing with reaviz, which is using reablocks Tooltip under the hood, the chart tooltips are all in the top left of the screen. I don't think any adjusting necessarily needs to be done to this PR, but I can't tell why it's happening so something to be aware of. The fix might need to be made here or in reaviz to get it all working together, it's unclear

Comment on lines +93 to +98
Object.assign(elementRef.current.style, {
width: 'max-content',
position: 'absolute',
top: 0,
left: 0
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh i bet this absolute position is the reaviz issue. Hopefully updating reaviz with some relative positioned parents or something would work

@amcdnl
Copy link
Member

amcdnl commented Jan 17, 2024

@shaswat-indian - We need to make sure we cover the line / area chart scenario in reaviz.

@shaswat-indian shaswat-indian marked this pull request as draft February 1, 2024 04:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants