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

Adds resize observer functionality #5963

Closed
wants to merge 17 commits into from

Conversation

jacwright
Copy link
Contributor

Implements ResizerObserver per @Rich-Harris #5524 (comment)

Done poorly, looking for guidance on doing this correctly or for someone else to take it on.

I started implementing this but it is stretching out too long. I haven't been in the code-base for a while and to add this a parameter needs to be added to the resize_observer function (the observer entries) so they can be set in the binding. I've got it working, but I believe my implementation is not clean. I wanted to publish what I had to save another time, but if I'm close then I'll fix it up with your direction.

//CC @pushkine @iamricard

Implements ResizerObserver per @Rich-Harris sveltejs#5524 (comment)

Done poorly, looking for guidance on doing this correctly or for someone else to take it on.
@benmccann benmccann marked this pull request as draft February 4, 2021 19:58
@jacwright
Copy link
Contributor Author

If nobody has a better way to implement this, should we call it good? Tests are passing. 🙂

@tanhauhau
Copy link
Member

tanhauhau commented Feb 27, 2021

looks okay to me in general. is there a test for this? eg: for bind:borderBoxSize etc

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Not sure what happened with that package.lock.json but I think its changed contents need to be omitted from this PR

@jacwright
Copy link
Contributor Author

Not sure what happened with that package.lock.json but I think its changed contents need to be omitted from this PR

Yeah, I reverted this. Good call. Thanks!

@jacwright jacwright marked this pull request as ready for review March 3, 2021 23:33
@jacwright jacwright marked this pull request as draft March 3, 2021 23:34
@dummdidumm
Copy link
Member

This would likely be a solution to #4233 and #6127

@RaiVaibhav
Copy link
Contributor

Hi Team, thanks for the svelte 🙌 and for everything.

I am new to the codebase, can anyone suggest to me what else needs to be done, so that I can take this MR from here and update the PR.

I have added the test on top of @jacwright commits and rebased it locally, @dummdidumm if there is no issue can I push that commit here for your review?

@205g0
Copy link

205g0 commented Oct 22, 2021

Any news on this?

@bluwy
Copy link
Member

bluwy commented Oct 22, 2021

@205g0 I've planned to bring this up in the next maintainer's meeting.

@Rich-Harris Rich-Harris marked this pull request as ready for review January 8, 2022 17:02
@bluwy
Copy link
Member

bluwy commented Jan 19, 2022

Just pushed some changes, but still a WIP. The bindings doesn't seem to work yet.

@bluwy bluwy marked this pull request as draft January 19, 2022 03:38
@bluwy
Copy link
Member

bluwy commented Jan 25, 2022

Found an issue with resize observer bindings. If we're using contentRect, contentBoxSize, borderBoxSize, devicePixelContentBoxSize, these values are only retrieved after the box is resized, or at least after the element is mounted. Hence, we can't have an initial value for these in render time, which is unexpected compared to bind:clientWidth and bind:clientHeight. Not sure how to handle this yet.

@jacwright
Copy link
Contributor Author

jacwright commented Jan 28, 2022

I read on MDN that contentRect may be deprecated in the future. Perhaps we leave that one out?

We could have 0 initial values until mount. clientWidth is 0 before mounting. We can set { inlineSize: 0, blockSize: 0 } as the initial value until mounting triggers the resize observer. And we can document it if needed. What do you think @bluwy?

And thank you for helping @bluwy!

@bluwy
Copy link
Member

bluwy commented Jan 29, 2022

I read on MDN that contentRect may be deprecated in the future. Perhaps we leave that one out?

I saw that too but left it in since it has better browser support, but then it would be confusing to coexist with contentBoxSize. I can remove contentRect for now and we can decide later if it's desirable.

We could have 0 initial values until mount. clientWidth is 0 before mounting. We can set { inlineSize: 0, blockSize: 0 } as the initial value until mounting triggers the resize observer. And we can document it if needed. What do you think @bluwy?

I'm not sure if this is safe since ResizeObserverSize has a constructor and us shimming it with n object could be risky. I think this would be more important for hydration since before mount (hydrate phase), the element already exist and it has a width and height. I believe the same goes to clientWidth too, which should be non-zero in the hydration-phase. (Untested)

I'll bring this up in the maintainer's meeting.

@bluwy
Copy link
Member

bluwy commented Jan 30, 2022

Update from the last maintainer's meeting, we had decided to leave the variable as undefined initially instead of shimming a default value. This would keep in line with how the "standards" work, though deviates from the behaviour of bind:clientWidth and bind:clientHeight, hence we should document this.

What's left on this PR is to clean it up, tests, and docs.

Copy link
Member

@tanhauhau tanhauhau left a comment

Choose a reason for hiding this comment

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

and yes, would love to see a unit test for this

Comment on lines +577 to +585
if (renderer.options.dev) {
block.chunks.mount.push(
b`${resize_observer} = @add_resize_observer_dev(${this.var}, ${callee}, { box: "${box}" });`
);
} else {
block.chunks.mount.push(
b`${resize_observer} = @add_resize_observer(${this.var}, ${callee}, { box: "${box}" });`
);
}
Copy link
Member

Choose a reason for hiding this comment

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

you dont have to do this here, any runtime function suffixed with _dev will be automatically used when the dev mode is true

Suggested change
if (renderer.options.dev) {
block.chunks.mount.push(
b`${resize_observer} = @add_resize_observer_dev(${this.var}, ${callee}, { box: "${box}" });`
);
} else {
block.chunks.mount.push(
b`${resize_observer} = @add_resize_observer(${this.var}, ${callee}, { box: "${box}" });`
);
}
block.chunks.mount.push(
b`${resize_observer} = @add_resize_observer(${this.var}, ${callee}, { box: "${box}" });`
);

observer.observe(node, opts);
return () => observer.disconnect();
}

Copy link
Member

Choose a reason for hiding this comment

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

i wonder if we could / should reuse the ResizeObserver instance

@bluwy
Copy link
Member

bluwy commented Apr 12, 2022

Thanks for the review @tanhauhau. I haven't been following up on this PR as well, so it has been stale for quite a while now. I'll try to pick it up soon, or if anyone feels like tackling this, feel free too.

@dummdidumm
Copy link
Member

Closes #7583

@searleser97
Copy link

are there plans to ever merge this PR? It looks like it would if the weird behavior of adding position: relative to style of element. #4776

@bluwy
Copy link
Member

bluwy commented Oct 29, 2022

@searleser97 Feel free to pick up the remaining work to complete the PR. There were discussions above that weren't implemented yet, and I haven't had the need for this feature to continue working on it 😬

@dummdidumm
Copy link
Member

Closing in favor #8022

@dummdidumm dummdidumm closed this Mar 14, 2023
dummdidumm added a commit that referenced this pull request Apr 11, 2023
Implements ResizeObserver bindings: #5524 (comment)
Continuation of: #5963
Related to #7583

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
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

8 participants