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

feat: add roundOffsets option to computeStyles modifier #1193

Merged
merged 1 commit into from Dec 14, 2020
Merged

feat: add roundOffsets option to computeStyles modifier #1193

merged 1 commit into from Dec 14, 2020

Conversation

piecyk
Copy link
Contributor

@piecyk piecyk commented Nov 17, 2020

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.

createPopper(reference, popper, {
  modifiers: [
    {
      name: 'computeStyles',
      options: {
        roundOffsets: false,
      },
    },
  ],
});

@rollingversions
Copy link

rollingversions bot commented Nov 17, 2020

Change Log for @popperjs/core (2.5.4 → 2.6.0)

New Features

  • Add roundOffsets option to computeStyles modifier

Edit changelog

@FezVrasta
Copy link
Member

Can't we fix the rounding algorithm instead? We have quite a lot of tests that cover it already.

@piecyk
Copy link
Contributor Author

piecyk commented Nov 17, 2020

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]) -
Copy link
Contributor Author

@piecyk piecyk Nov 17, 2020

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@atomiks? 👀

Copy link
Contributor Author

@piecyk piecyk Dec 1, 2020

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.

Copy link
Member

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 :-)

Copy link
Contributor Author

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 🤞 💪

src/utils/computeOffsets.js Outdated Show resolved Hide resolved
@piecyk
Copy link
Contributor Author

piecyk commented Nov 18, 2020

Don't really see a way to improve rounding algorithm to be more accurate 🤷‍♂️ Maybe till then we can have this opt out option.

@piecyk piecyk changed the title feat: add round option to popperOffsets, computeOffsets, default true feat: add roundOffsets option to computeStyles modifier Nov 18, 2020
@piecyk
Copy link
Contributor Author

piecyk commented Nov 19, 2020

Changed this to keep the rounding logic in computeStyles modifier, added roundOffsets option as boolean to modifier options as in PR description. Was wondering also if not allow to pass custom function, so then user of modifier can tweak the rounding.

@FezVrasta FezVrasta merged commit 24cb3a8 into floating-ui:master Dec 14, 2020
@FezVrasta
Copy link
Member

Merged, thanks for the help! Could you also send a PR on the computeStyles docs? here

@piecyk
Copy link
Contributor Author

piecyk commented Dec 14, 2020

Could you also send a PR on the computeStyles docs?

Yes, will do.

@piecyk
Copy link
Contributor Author

piecyk commented Dec 14, 2020

@FezVrasta when are you planing releasing this? I think patch release will be good in this case. Also thanks for this 👏

@FezVrasta
Copy link
Member

Releasing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants