Skip to content

Add option for different types of dimensions #31

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

Closed
wants to merge 1 commit into from

Conversation

TyMick
Copy link

@TyMick TyMick commented Mar 10, 2020

Hi, @ZeeCoder! Came across your project looking for a hook that would keep track of changes to element offsetWidth/offsetHeight. I love your implementation of the ResizeObserver API, and I ended up finding the API can track offset, client, and scroll dimensions as well via ResizeObserverEntry.target.

What would you think of adding one more option called something like type, leaving "content" as default but adding the option for "offset", "client", and "scroll" as well? This edit's been tracking offsetWidth and offsetHeight perfectly for me so far.

Hi, @ZeeCoder! Came across your project looking for a hook that would keep track of changes to element `offsetWidth`/`offsetHeight`. I love your implementation of the `ResizeObserver` API, and I ended up finding the API can track offset, client, and scroll dimensions as well via `ResizeObserverEntry.target`.

What would you think of adding one more option called something like `type`, leaving `"content"` as default but adding the option for `"offset"`, `"client"`, and `"scroll"` as well? This edit's been tracking `offsetWidth` and `offsetHeight` perfectly for me so far.
@mattlucock
Copy link

mattlucock commented Mar 10, 2020

Your "offset" option is already in the spec; it's just badly supported at the moment.

My feeling is that this hook should just be a thin abstraction over the ResizeObserver API, and shouldn't return any data not returned by the observer itself. If you want to track clientWidth or scrollWidth (or offsetWidth, for the time being) etc., you could do so very easily by passing in your own ref and using a onResize callback.

@mattlucock
Copy link

Perhaps the hook could have an option to track borderBoxSize now and it actually just tracks offsetWidth and offsetHeight until there's better browser support. That would actually be quite useful.

@ZeeCoder
Copy link
Owner

Sorry for not reacting to this yet, but as there's a lot going on in my life in light of recent events I can't dedicate the time to this it would deserve.
Thank you both for your efforts, I'll get back to you as soon as I'm able.

@ZeeCoder
Copy link
Owner

So I can imagine exposing the entry in one way or another, but I'd like to keep the current interface as simple as possible.

So for example I wouldn't return the entry in the hook response, because I would need to store that in a hook state, which means the hook would trigger a re-render every time there's a new entry, regardless of whether you were interested in the entry or not.

Instead, I would add an observation option forwarded to RO.observe, and then maybe a handleEntry callback, so something like this:

// Notice I omit width/height here, as the hook would omit them,
// similar to when onResize is provided
const { ref } =  useResizeObserver<HTMLDivElement>({
  observeOptions: { box : 'border-box' },
  handleEntry: (entry: ResizeObserverEntry) => { ... },
});

I'd also need to make sure onResize is still usable alongside it though, which also means more tests for such edge cases, and it'll require more TS function overloads too.

The more options you have the more complicated the internals become, as you need to support all combinations. 😅

The good thing about all of this is that it makes the hook more future proof, as now there'd be a way to receive the ResizeObserver entry as-is.

@ZeeCoder
Copy link
Owner

Or maybe I could simply augment the onResize callback, where it could receive the entry itself as well actually.

@ZeeCoder
Copy link
Owner

What you say @matthewlucock is exactly right, I'd like to keep the hook as lean as possible.
That's the reason I implemented onResize instead of explicit debounce / throttle support within the options, as it just leads down a rabbit hole of options to support.

It's a lot easier to just compose new hooks out of this one as a base, especially if the entry is exposed, then it becomes very easy to implement what @tywmick would like.

I might consider shipping such higher-level hooks alongside the main one though in the future, if there's demand.

@mattlucock
Copy link

I see how providing the actual entry itself serves to future-proof the hook in principle, but I'm not sure if that would actually be useful. Tracking the content box and the border box are the only two things that ResizeObservers can do, and, y'know, I can't really think of anything else they could possibly do in the future—the content box and the border box covers it. Tracking the border box this way definitely wouldn't provide the elegance of the existing API of this hook. As for composing this hook to track scrollWidth/scrollHeight or clientWidth/clientHeight or whatever else, I don't see that being necessary to support. I think that doing what I said—passing in a ref and using the onResize callback—is a much more elegant solution for that use case.

I no longer actually have a use case for any of this because I'm no longer in the job where I was using this hook, but I definitely did have a use case for tracking the border box. I just did it using this hook by wrapping the component I was trying to track in a simple <div>, because in that case, the content box of the parent is the border box of the child.

@ZeeCoder
Copy link
Owner

Fair points

@ZeeCoder ZeeCoder mentioned this pull request May 21, 2020
@ZeeCoder
Copy link
Owner

For reference, these are the entry properties:
https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserverEntry

Currently the hook uses contentRect.width and -height, and ignores the rest.

@drizzlesconsin
Copy link

any updates?

@ZeeCoder
Copy link
Owner

@drizzlesconsin Sorry, but nothing apart from this: #46 (comment)

ZeeCoder added a commit that referenced this pull request Aug 28, 2021
@ZeeCoder ZeeCoder closed this in f873597 Aug 28, 2021
github-actions bot pushed a commit that referenced this pull request 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 pull request Aug 28, 2021
ZeeCoder pushed a commit that referenced this pull request 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 pull request 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
@TyMick TyMick deleted the patch-1 branch September 1, 2021 02:55
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 this pull request may close these issues.

4 participants