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

rounding causes trouble... #46

Closed
jedierikb opened this issue Oct 7, 2020 · 9 comments
Closed

rounding causes trouble... #46

jedierikb opened this issue Oct 7, 2020 · 9 comments
Labels

Comments

@jedierikb
Copy link

jedierikb commented Oct 7, 2020

Rounding the returned dimensions can cause trouble.

For example, I am using your observer to measure an element so I can fit a canvas element inside.
When the available space is fractional, the observer currently rounds up, and my resultant canvas is larger than its parent. Alas - unwanted scrollbars can result!

Maybe consider changing to returning the values without rounding?

@ZeeCoder
Copy link
Owner

hmm, interesting edge case.
I'm currently using "Math.round" (not rounding up), which should be how CSS resolves subpixel values.

The hook is also specifically done in a way so that it doesn't report every tiny subpixel change, which is why the rounding is in place to begin with.

Would having a way to opt out of rounding work for you?

Just note that having something like that would mean that your hosting component will get rerendered on each subpixel change.
(Unless you'd use the onResize function, in which case you could round the values however you like, and only trigger a render when necessary.)

@ZeeCoder ZeeCoder added the enhancement New feature or request label Oct 11, 2020
@IgnusG
Copy link

IgnusG commented Nov 23, 2020

I've also just stumbled upon this. The actual size of my element is 82.4px with a tiny overflow - useResizeObserver returns 82px which cuts off the element:

image

image

In this case, I wouldn't mind opting into using Math.ceil instead of Math.round - maybe a round option which accepts a function could be made available?
By default it could just use Math.round and everyone could either pass in Math.ceil, Math.floor or a custom function (for exact cases just an identity fn (a) => a would turn all rounding off and "opt in" to subpixel re-rendering).

What's your opinion on this @ZeeCoder @jedierikb?

@ZeeCoder
Copy link
Owner

Hmm, yeah I see the issue here.
Obviously rounding was put in place so that your component does not rerender on every subpixel change, but I see how it can cause issues.

I kinda thought that when people wanted to opt out of the default rounding behaviour, then they can just use the onResize callback.
However I understand that that would be more work, as you'd have to track size state then yourself in a custom hook, and opt out of rerenders when your custom rounding function produces the same value that's set at the time.

It shouldn't be too hard to add support for a custom function, and I don't see any immediate issues with it either.
Could be one that takes in both values at the same time, basically "processing" the reported values:
(w, h) => [newW, newH];

Then you could not only process the values differently, but do other fancy things like keeping an aspect ratio:
(w, h) => [w, w * 1.5]

Not sure how useful that is though.

@jedierikb
Copy link
Author

I like the idea of passing in a custom function :-)

@vaniya-k
Copy link

vaniya-k commented May 18, 2021

@ZeeCoder first of all, many thanks for this tiny and handy library!

i do have issues though in my production project with rounding.. is there a preliminary ETA for merging the fix (i'm looking at #55)?

thank you in advance!

@ZeeCoder
Copy link
Owner

Hi @vaniya-k ,

I'm glad you find the lib useful, that means a lot, thank you. 🙏

I'm really sorry but I can't really promise anything for the nearby future, as I'm expecting my daughter to be born in mid June and I can't justify the time spent on the project right now.

I do have a bigger update that'd clear out all the pending issues / PRs, but I'm just not sure I'll get the right time for it.

For the time being I'd recommend implementing your own wrapper hook, which would implement onResize with local state, so that you can do the rounding any way you like.

Then once this lib would come out with a fix, you could just replace your internal implementation with a direct useResizeObserver call for a seemless transition.

@vaniya-k
Copy link

@ZeeCoder

oh boy, you do have much more on your plate than an average open source creator / maintainer)) wish you all the best with the upcoming addition to your family!))

no worries, we have a good-enough workaround, just wanted to inquiry on the roadmap

@ZeeCoder
Copy link
Owner

Thanks, really appreciate it 🙏

ZeeCoder added a commit that referenced this issue Aug 28, 2021
ZeeCoder added a commit that referenced this issue Aug 28, 2021
ZeeCoder added a commit that referenced this issue Aug 28, 2021
github-actions bot pushed a commit that referenced this issue Aug 28, 2021
# [7.1.0](v7.0.1...v7.1.0) (2021-08-28)

### Bug Fixes

* The `onResize` callback is no longer incorrectly called with the same values. ([29938a1](29938a1))

### Features

* Added the `box` option ([f873597](f873597)), closes [#31](#31) [#57](#57)
* Added the `round` option. ([1224bc8](1224bc8)), closes [#55](#55) [#46](#46) [#61](#61)
ZeeCoder added a commit that referenced this issue Aug 28, 2021
ZeeCoder pushed a commit that referenced this issue Aug 28, 2021
# [7.1.0](v7.0.1...v7.1.0) (2021-08-28)

### Bug Fixes

* The `onResize` callback is no longer incorrectly called with the same values. ([29938a1](29938a1))

### Features

* Added the `box` option ([f873597](f873597)), closes [#31](#31) [#57](#57)
* Added the `round` option. ([1224bc8](1224bc8)), closes [#55](#55) [#46](#46) [#61](#61)
github-actions bot pushed a commit that referenced this issue Aug 28, 2021
# [8.0.0](v7.0.1...v8.0.0) (2021-08-28)

### Bug Fixes

* Removed `resize-observer-polyfill` in favour of `@juggle/resize-observer`. ([8afc8f6](8afc8f6))
* The `onResize` callback is no longer incorrectly called with the same values. ([bd0f3c8](bd0f3c8))

### Features

* Added the `box` option ([0ca6c23](0ca6c23)), closes [#31](#31) [#57](#57)
* Added the `round` option. ([aa38199](aa38199)), closes [#55](#55) [#46](#46) [#61](#61)
* Triggering the v8 release ([4373a1f](4373a1f))

### BREAKING CHANGES

* Triggering the v8 release, which was published as 7.1.0 by accident.
@github-actions
Copy link

🎉 This issue has been resolved in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Repository owner deleted a comment from github-actions bot Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants