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

[v5] [core] feat: remove @juggle/resize-observer in favor of native API #6234

Merged
merged 3 commits into from
Jun 22, 2023

Conversation

jkillian
Copy link
Contributor

Fixes #6233

Checklist

  • Includes tests (just the existing ones)
  • Update documentation

Changes proposed in this pull request:

Removes the use of the @juggle/resize-observer polyfill in favor of directly using the native API.

Reviewers should focus on:

Is the code to support isomoprhic rendering okay?

Screenshot

N/A

@@ -72,7 +71,8 @@ export class ResizeSensor extends AbstractPureComponent<ResizeSensorProps> {

private prevElement: HTMLElement | undefined = undefined;

private observer = new ResizeObserver(entries => this.props.onResize?.(entries));
private observer =
globalThis.ResizeObserver != null ? new ResizeObserver(entries => this.props.onResize?.(entries)) : undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure if this is the best way to make things work correctly in an isomorphic setting, but it seemed reasonable to me and got the tests to pass locally

Copy link
Contributor

Choose a reason for hiding this comment

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

seems fine

@jkillian jkillian marked this pull request as ready for review June 22, 2023 18:55
@adidahiya
Copy link
Contributor

Nice, this sounds like a good idea. Just in time for v5.0, I'm about to cut the stable release this week.

I'm working on the next branch -- I'll try to update your PR to target that one.

@adidahiya adidahiya changed the base branch from v5 to next June 22, 2023 19:26
@adidahiya
Copy link
Contributor

Additional note: it sounds like the caveats listed here in "Switching between native and polyfilled versions" don't apply to us since we don't actually use any of the ResizeEntry data in Blueprint components.

@adidahiya adidahiya changed the title Remove use of @juggle/resize-observer in favor of native API [v5] [core] feat: remove @juggle/resize-observer in favor of native API Jun 22, 2023
@adidahiya adidahiya merged commit 41d5874 into palantir:next Jun 22, 2023
@jkillian
Copy link
Contributor Author

Oops, sorry for targeting the wrong branch, thanks for taking care of that and merging!

@jkillian jkillian deleted the jk/remove-resize-observer-polyfill branch June 22, 2023 21:14
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.

2 participants