Skip to content

[EuiResizeObserver] Remove compatibility fallback#4709

Merged
thompsongl merged 6 commits intoelastic:masterfrom
thompsongl:4402-resizeobserver
Apr 15, 2021
Merged

[EuiResizeObserver] Remove compatibility fallback#4709
thompsongl merged 6 commits intoelastic:masterfrom
thompsongl:4402-resizeobserver

Conversation

@thompsongl
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl commented Apr 13, 2021

Summary

Part of #4402. Removes makeCompatibleObserver fallback originally written for a time when EUI's supported browsers did not all implement the ResizeObserver API.

Note that the tests have been skipped:

The makeCompatibleObserver fallback method is no longer necessary.

This is true, except for jest environments. jsdom does not implement the ResizeObserver API, which means that jest is using the MutationObserver fallback in makeCompatibleObserver. It appears that resize-observer-polyfill often recommended for jest environments uses a similar MutationObserver fallback approach.
So, by removing our MutationObserver fallback and relying solely on ResizeObserver, we'll lose the ability to test EuiResizeObserver and useResizeObserver. This seems fine, because our supported browsers all implement the ResizeObserver API, which means our tests were not actually testing the real, intended behavior of target browsers. (#4402 (comment))

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles

- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_4709/

Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; I'm not sure the @types/resize-observer-browser will get picked up automatically by downstream projects / not cause some other issue in Kibana, would you mind doing a quick pass there to verify?

@thompsongl
Copy link
Copy Markdown
Contributor Author

I'm not sure the @types/resize-observer-browser will get picked up automatically by downstream projects

Good call. I questioned whether to use the lib at all. I'll test it but might just bring the types we need inline.

@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_4709/

@thompsongl
Copy link
Copy Markdown
Contributor Author

Types do get picked up in downstream projects. Tested in CRA and Kibana.

Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Thanks for double checking the impact! Changes LGTM

@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_4709/

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.

3 participants