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

fix: use ResizeObserver instead of window resize event listener #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obecker
Copy link
Contributor

@obecker obecker commented Nov 5, 2024

Fixes #33

src/index.ts Outdated
Comment on lines 99 to 103
if (elementRef.current != null) {
const observer = new ResizeObserver(debounceFn(clampLines, debounce))
observer.observe(elementRef.current)
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.

Thought: maybe we could move observer to a let outside of this if statement, and move the return statement outside of the if? And then do something like observe?.disconnect().

So we're sure we always disconnect even if the component happens to unmount. Shouldn't, but just to be on the safe side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I understand your concern. Either the observer is created, then a cleanup function is returned that disconnects it when unmounted. Or this block is not executed (there is no observer), then no cleanup function is returned. So we are on the safe side. I guess it's more a question of readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rewritten the code to use an early return. I think that way it is more clear what is happening.
(By the way, I believe we could move that early return even before the clampLines() call in line 97. However, I'm not sure if I miss something ...)

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.

Resize handling
2 participants