-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: add roundOffsets
option to computeStyles modifier
#1193
Conversation
Change Log for @popperjs/core (2.5.4 → 2.6.0)New Features
|
Can't we fix the rounding algorithm instead? We have quite a lot of tests that cover it already. |
I think it will be hard to make a fix that will work cross browser. Also while testing locally latest chrome/firefox/edge/safari with opting out, didn't break the rendering as mainly that's the reason for rounding. |
@@ -68,17 +84,15 @@ export default function computeOffsets({ | |||
switch (variation) { | |||
case start: | |||
offsets[mainAxis] = | |||
Math.floor(offsets[mainAxis]) - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we had this round functions on start/end variation? Imho they are not needed as latter we still round the offsets 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@atomiks? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should add more tests 🤔 Also this change didn't break any existing one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good, I'm just a bit worried about this bit. Let's give @atomiks a couple more days to review it and if he doesn't find time I'll merge it as dog food it to the whole user base :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like no other comments. IMHO let's merge this 🤞 💪
Don't really see a way to improve rounding algorithm to be more accurate 🤷♂️ Maybe till then we can have this opt out option. |
round
option to popperOffsets, computeOffsets, default trueroundOffsets
option to computeStyles modifier
Changed this to keep the rounding logic in computeStyles modifier, added |
Merged, thanks for the help! Could you also send a PR on the |
Yes, will do. |
@FezVrasta when are you planing releasing this? I think patch release will be good in this case. Also thanks for this 👏 |
Releasing |
As reported in #1169 rounding logic in some cases makes the popper not correctly aligned with reference element. This PR adds possibility to opt out from it via roundOffsets option.