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

Uses ResizeObserver on resize listeners if available #5885

Closed
wants to merge 1 commit into from

Conversation

iamricard
Copy link

@iamricard iamricard commented Jan 15, 2021

As discussed in #4233, ResizeObserver is now widely available (~90% as of 15/01/2021, source) so we can use it instead of a custom-built solution to listen to resizes.

I also needed to add @types/resize-observer-browser because the type definition for ResizeObserver hasn't landed in TS yet (see microsoft/TypeScript#28502).

Closes #4233.

Testing:

As discussed in sveltejs#4233, ResizeObserver is now widely available (~90% as
of 15/01/2021, [source](http://caniuse.com/?search=ResizeObserver)) so
we can use it instead of a custom-built solution to listen to resizes.

I also needed to add @types/resize-observer-browser because the type
definition for `ResizeObserver` hasn't landed in TS yet (see
microsoft/TypeScript#28502).

Closes sveltejs#4233.
@iamricard
Copy link
Author

iamricard commented Jan 15, 2021

I might be wrong, but the failure looks like a flake 🤔

@porfirioribeiro
Copy link

I wonder if there could be some sort of flag to allow the old implementation not be included at all?
Your target might be on that 90% so you would not need to have that impl

@iamricard
Copy link
Author

@porfirioribeiro that sounds like a great idea! It was already suggested in #4233 by @Prinzhorn. I'd be happy to work on it too but it sounds like it would be better to do that in a separate PR.

The only caveat is I'm not sure if there's a precedent for this kind of feature (this sounds like we'd need to add a new option to the compiler). If there isn't a feature like it then I'm not convinced it's worth the extra complexity it might add to the compiler config consider how little we stand to gain from it (hopefully someone from the Svelte team can chime in).

@pushkine
Copy link
Contributor

duplicate #5524

@benmccann
Copy link
Member

It seems like this PR wouldn't be accepted as is for the reason mentioned in #5524 (comment). We'd have to come up with a solution to Rich's concern there. I'm going to close this to keep the discussion in a single place

@benmccann benmccann closed this Jan 18, 2021
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.

bind:offsetHeight doesn't update in certain cases
4 participants