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

Fixed issue with EuiResizeObserver fallback #3088

Merged
merged 7 commits into from
Mar 23, 2020
Merged

Fixed issue with EuiResizeObserver fallback #3088

merged 7 commits into from
Mar 23, 2020

Conversation

PeterSenpai
Copy link
Contributor

@PeterSenpai PeterSenpai commented Mar 16, 2020

Summary

Fixes #3044

The cause was mentioned in the issue.

Basically, Safari and IE11 do not support ResizeObserver so that they won't catch the window resizing events properly.

The fix is to add resize event listener to them.

Before:
ie12_before

Safari_before (2)

After:

ie12_fix

safari_fix (2)

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cchaos cchaos requested a review from thompsongl March 18, 2020 00:52
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thanks, @PeterSenpai

This does address 1 of 2 points made in the issue. The second point ("confirm that a newly computed size is, in fact, different from the previously known one") is just as important as the resize listener, and we will want to accomplish it as part of this fix.

Also, as this is a bugfix, you'll need to add a changelog entry (see: https://github.com/elastic/eui/blob/master/CONTRIBUTING.md#documentation)

@PeterSenpai
Copy link
Contributor Author

PeterSenpai commented Mar 18, 2020

Thanks, @PeterSenpai

This does address 1 of 2 points made in the issue. The second point ("confirm that a newly computed size is, in fact, different from the previously known one") is just as important as the resize listener, and we will want to accomplish it as part of this fix.

Also, as this is a bugfix, you'll need to add a changelog entry (see: https://github.com/elastic/eui/blob/master/CONTRIBUTING.md#documentation)

Thank you @thompsongl for the feedback!
I updated the changelog. As for your first concern, I thought it already did in the code? In src/components/observer/resize_observer.tsx line 77

  const _currentDimensions = useRef(size);
  const setSize = useCallback(dimensions => {
    if (
      _currentDimensions.current.width !== dimensions.width ||
      _currentDimensions.current.height !== dimensions.height
    ) {
      _currentDimensions.current = dimensions;
      _setSize(dimensions);
    }
  }, []);

I might misundsertand the 2nd point and can totally be wrong.

@thompsongl
Copy link
Contributor

jenkins test this

As for your first concern, I thought it already did in the code?

You are correct! 😅 This is looking good.

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor

jenkins test this

@cchaos cchaos changed the title Fix the resizing issue with certian browser Fixed issue with EuiResizeObserver fallback Mar 19, 2020
@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Upon further investigation, the code in src/components/observer/resize_observer.tsx line 77 does not apply to the standard EuiResizeObserver method, only the useResizeObserver hook.
So, we still need to account for cases where the callback is not the result of an actual resize event.

@PeterSenpai
Copy link
Contributor Author

Upon further investigation, the code in src/components/observer/resize_observer.tsx line 77 does not apply to the standard EuiResizeObserver method, only the useResizeObserver hook.

So, we still need to account for cases where the callback is not the result of an actual resize event.

Thanks! I will take a look.

@PeterSenpai
Copy link
Contributor Author

Hi, @thompsongl. I added the logic that would check if the size is actually changed.

There might be a cleaner way to do it if the function makeResizeObserver(line 69) doesn't have to return a MutationObserver.

Also when I did filter out the non-resize events, the EuiResizeObserver test failed. I found out that there's a customized getBoundingClientRect() before useResizeObserver but not EuiResizeObserver and the reason of fail tests is that no matter how the test modifies the node the size of the node is always 0. Thus, I refactor the test a bit so that both EuiResizeObserver and useResizeObserver share the same getBoundingClientRect() and the tests passed.

Please give me feedback on my approach and what needs to be changed.

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

There might be a cleaner way to do it if the function makeResizeObserver(line 69) doesn't have to return a MutationObserver.

Yeah, but we do need to support the MutationObserver fallback method. Your current code looks good 👍

Testing updates look good, also

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.

EuiResizeObserver fallback does not work for pure resizing
3 participants