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

[useResizeObserver] Fix initial width and height being 0 in docs example #7518

Merged
merged 4 commits into from
Feb 8, 2024
Merged

[useResizeObserver] Fix initial width and height being 0 in docs example #7518

merged 4 commits into from
Feb 8, 2024

Conversation

Juneezee
Copy link
Contributor

@Juneezee Juneezee commented Feb 7, 2024

Summary

Fixes #7458.

Old PR description

In the useResizeObserver hook, we set the initial state of width and height as 0:

const [size, _setSize] = useState({ width: 0, height: 0 });

This is okay because we have a useEffect that will trigger a re-render when the container has changed:

useEffect(() => {
if (container != null) {
// ResizeObserver's first call to the observation callback is scheduled in the future
// so find the container's initial dimensions now
const boundingRect = container.getBoundingClientRect();
setSize({
width: boundingRect.width,
height: boundingRect.height,
});
const observer = makeResizeObserver(container, () => {
// `entry.contentRect` provides incomplete `height` and `width` data.
// Use `getBoundingClientRect` to account for padding and border.
// https://developer.mozilla.org/en-US/docs/Web/API/DOMRectReadOnly
const { height, width } = container.getBoundingClientRect();
setSize({
width,
height,
});
});
return () => observer && observer.disconnect();
} else {
setSize({ width: 0, height: 0 });
}
}, [container, setSize]);

We are calling our customized setSize in the useEffect. This custom setSize only calls the _setSize (from useState) when the new and old dimension differs.

const setSize = useCallback(
(dimensions: { width: number; height: number }) => {
const doesWidthMatter = dimension !== 'height';
const doesHeightMatter = dimension !== 'width';
if (
(doesWidthMatter &&
_currentDimensions.current.width !== dimensions.width) ||
(doesHeightMatter &&
_currentDimensions.current.height !== dimensions.height)
) {
_currentDimensions.current = dimensions;
_setSize(dimensions);
}
},
[dimension]
);

To fix the bug, we simply set the initial width and height as undefined. Then, the else block in useEffect will "initialise" the width and height to 0. Now setSize detects that the old and new dimension differs (undefined to 0), so it will update the state and trigger a re-render.

} else {
setSize({ width: 0, height: 0 });
}

As a result, useResizeObserver will now return the initial dimension correctly.

This solution is inspired by: https://github.com/ZeeCoder/use-resize-observer/blob/465fbc11c92e1b97a1dcb526b44ce4d889956951/src/index.ts#L62-L68

QA

  1. Go to Resize observer page
  2. useResizeObserver now returns the same height and width as the EuiResizeObserver example
2024-02-07.23-51-46.mp4

General checklist

Copy link

github-actions bot commented Feb 7, 2024

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

Signed-off-by: Eng Zer Jun <[email protected]>
@Juneezee Juneezee marked this pull request as ready for review February 7, 2024 15:57
@Juneezee Juneezee requested a review from a team as a code owner February 7, 2024 15:57
height: {height}; width: ${width}
height: {height}; width: {width}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes the $ typo

image

@cee-chen
Copy link
Contributor

cee-chen commented Feb 7, 2024

Hey @Juneezee! Thanks so much for this PR, it's really helped me realize what the underlying problem is.

Rather than fixing the useResizeObserver source code, I think we need to change the documentation example to tell consumers to use useState instead of useRef. The issue is that our useEffect isn't registering a container update (due to the fact that it's a ref) when it should. Changing

const resizeRef = useRef();
const dimensions = useResizeObserver(resizeRef.current);

to

const [resizeRef, setResizeRef] = useState();
const dimensions = useResizeObserver(resizeRef);

// ...

<div className="eui-displayInlineBlock" ref={setResizeRef}>

Also solves the issue of returning the correct width/height on mount without requiring any source code change. I think I'd prefer this approach (having consumers solve it) rather than us adding an extra rerender. This allows consumers to opt in to useRef instead if they prefer to only receive size dimensions on update rather than on mount.

What do you think?

@Juneezee
Copy link
Contributor Author

Juneezee commented Feb 8, 2024

@cee-chen Thanks for reviewing this PR! Your suggestion looks great to me. I have updated the PR, please take a look again. Thanks 😄 !

@Juneezee Juneezee changed the title [useResizeObserver] Fix initial width and height being 0 [useResizeObserver] Fix initial width and height being 0 in docs example Feb 8, 2024
@cee-chen cee-chen added documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog labels Feb 8, 2024
@cee-chen
Copy link
Contributor

cee-chen commented Feb 8, 2024

buildkite test this

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Thanks again @Juneezee, you're amazing!! 🚀

@cee-chen cee-chen merged commit 46d5138 into elastic:main Feb 8, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community contribution documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align useResizeObserver with EuiResizeObserver component behavior
4 participants