Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Fix issue with preview width when window is resized #109

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

mike-day
Copy link
Contributor

@mike-day mike-day commented Mar 13, 2023

Currently, in the patterns view, the previews look a bit odd when the browser window is resized:

pm-window-resize-issue

The previews width effectively remain static, but they should be updating as the window size changes.


This PR triggers a fresh render of the PatternGrid when the window is resized, causing the previews to be rendered at the expected width:

pm-window-resize-fix


How to test

  1. Checkout the branch
  2. In the main patterns view, try resizing your browser window
  3. The previews should update to remain at the expected width as the window size changes

Comment on lines +32 to +34
// If the window is resized, trigger a fresh render of the grid.
// Helps ensure PatternPreview iFrames are the right size.
useWindowResize();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

useWindowResize is called in PatternGrid so only the grid is re-rendered (instead of calling higher in the tree and also re-rendering categories, for example).

Copy link
Contributor

@kienstra kienstra Mar 14, 2023

Choose a reason for hiding this comment

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

Really good idea to keep re-rendering to this low scope


/** Rerender the calling component when the window is resized. */
export default function useWindowResize() {
const [ , setWindowSize ] = useState( [ 0, 0 ] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also add a windowSize const and return it if we wanted to use that data elsewhere, but I see no practical usage for it right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a nice trick to force a re-render.

The fact that there's a no windowSize makes it clear that we're not using it, we're just using setState() to force a re-render.

Comment on lines -31 to +30
useEffect( () => {
if ( previewContainer?.current ) {
setPreviewContainerSize(
previewContainer?.current?.getBoundingClientRect()
);
}
}, [ previewContainer ] );
const previewContainerSize: BoundingClientRect | undefined =
previewContainer?.current?.getBoundingClientRect();
Copy link
Contributor Author

@mike-day mike-day Mar 13, 2023

Choose a reason for hiding this comment

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

Derived state needs to be used for previewContainerSize for this idea to work.

With it, a fresh getBoundingClientRect() is called each time the component is rendered.

Otherwise, useEffect is not called again after initial render since the previewContainer dependency does not update, making it harder to call setPreviewContainerSize with the new bounding client rect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to use derived state

@mike-day mike-day mentioned this pull request Mar 13, 2023
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Hi @mike-day,
Really nice way of getting the iframes to resize!

This works well:

iframe-resize

Comment on lines -31 to +30
useEffect( () => {
if ( previewContainer?.current ) {
setPreviewContainerSize(
previewContainer?.current?.getBoundingClientRect()
);
}
}, [ previewContainer ] );
const previewContainerSize: BoundingClientRect | undefined =
previewContainer?.current?.getBoundingClientRect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to use derived state

Comment on lines +32 to +34
// If the window is resized, trigger a fresh render of the grid.
// Helps ensure PatternPreview iFrames are the right size.
useWindowResize();
Copy link
Contributor

@kienstra kienstra Mar 14, 2023

Choose a reason for hiding this comment

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

Really good idea to keep re-rendering to this low scope

export default function useWindowResize() {
const [ , setWindowSize ] = useState( [ 0, 0 ] );

useLayoutEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of useLayoutEffect() instead of useEffect()

@dreamwhisper dreamwhisper mentioned this pull request Mar 14, 2023
11 tasks
Copy link
Contributor

@dreamwhisper dreamwhisper left a comment

Choose a reason for hiding this comment

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

Looks great! I added the PR link to our beta list on #57 and checked it off!

@mike-day
Copy link
Contributor Author

Thanks for the checking this out @kienstra and @dreamwhisper!

@mike-day mike-day merged commit b62a80a into main Mar 14, 2023
@mike-day mike-day deleted the fix/window-resize-wrong-preview-width branch March 14, 2023 15:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants