Fix preview width when searching or filtering patterns #115

mike-day · 2023-03-14T20:19:30Z

Related to the work in #109 and #95 — preview widths are sometimes stale when filtering by clicking category options or searching in the patterns view:

The width expands when it is the only filtered option, but remains at full-width when it should be resized to accommodate a second result.

This PR expands on and replaces the useWindowSize hook in #109 to allow other dependencies (such as filteredPatterns/themePatterns) to also trigger a re-render:

Now, regardless of how the user searches, clicks through categories, or resizes the window, we can also be sure a fresh grid of pattern previews with correct dimensions is displayed.

How to test

Checkout the branch
Filter the patterns by searching and/or clicking various categories
Resize your browser window
The previews should remain at the expected scale throughout

Notes

Not that this was expected to change, but even with the extra rendering introduced by these changes, Lighthouse scores remain screaming fast. Care was taken to ensure this does not cause an unneeded re-render on initial load:

mike-day · 2023-03-14T20:27:34Z

wp-modules/app/js/src/components/Patterns/PatternGrid.tsx

 	// Helps ensure PatternPreview iFrames are the right size.
-	useWindowResize();
+	useForceRerend( [ themePatterns ] );


The main problem relates to not being certain we are getting the correct BoundingClientRect (and thus scale) in PatternPreview, even when new categories are filtered.

This was the simplest way I could think of to be absolutely sure all previews in the grid are freshly rendered with the correct BoundingClientRect while using a derived state for previewContainerSize.

The patterns passed via themePatterns are filtered by search term or selected category, so we can use it instead of passing any new props.

This is a good way to force rerendering when the filtered patterns change.

mike-day · 2023-03-14T20:32:15Z

wp-modules/app/js/src/hooks/useForceRerend.ts

+export default function useForceRerend< T extends unknown >(
+	dependencies: T[] = []
+) {


I think it is okay to allow pretty much any type of value in the dependencies array.

Yeah, useForceRerend() is agnostic to the component that calls it.

mike-day · 2023-03-14T20:33:19Z

wp-modules/app/js/src/hooks/useForceRerend.ts

Refactoring my solution from yesterday here 🤣

Sounds good, it's always good to look back and improve things

mike-day · 2023-03-14T20:34:10Z

wp-modules/app/js/src/hooks/useForceRerend.ts

+		// Do not cause a re-render on first render.
+		if ( initialRender.current ) {
+			initialRender.current = false;
+		} else {
+			setForceUpdate( dependencies );
+		}


This setForceUpdate() should only run when dependencies are updated.

Do you see a performance boost compared to not using the if guard clause?

diff --git a/wp-modules/app/js/src/hooks/useForceRerend.ts b/wp-modules/app/js/src/hooks/useForceRerend.ts index b8655ffc..59db5d80 100644 --- a/wp-modules/app/js/src/hooks/useForceRerend.ts +++ b/wp-modules/app/js/src/hooks/useForceRerend.ts @@ -11,21 +11,12 @@ export default function useForceRerend< T extends unknown >( dependencies: T[] = [] ) { const [ , setForceUpdate ] = useState< T[] | WindowDimensions >(); - const initialRender = useRef( true ); useLayoutEffect( () => { - // Do not cause a re-render on first render. - if ( initialRender.current ) { - initialRender.current = false; - } else { - setForceUpdate( dependencies ); - } + setForceUpdate( dependencies );

I'd vote to keep this if/else only if it adds a big performance boost.

Otherwise, simply calling setForceUpdate( dependencies ); might be fine.

In testing, it does not really improve performance in any meaningful way (especially considering that we are potentially get just one additional state update on initial render — it's a non-issue right now).

That block and ref were removed in 0884c93!

Thanks, 0884c93 looks really good.

kienstra

Hi @mike-day,
Nice work! A few implementation comments below, but this is a nice improvement.

kienstra · 2023-03-14T21:35:06Z

Looks like my comments didn't show up in the review. Adding some now.

kienstra

Here are the comments I meant to leave before

wp-modules/app/js/src/hooks/useForceRerend.ts

kienstra · 2023-03-14T21:37:27Z

wp-modules/app/js/src/components/Patterns/PatternGrid.tsx

 	// Helps ensure PatternPreview iFrames are the right size.
-	useWindowResize();
+	useForceRerend( [ themePatterns ] );


This is a good way to force rerendering when the filtered patterns change.

wp-modules/app/js/src/hooks/useForceRerend.ts

kienstra · 2023-03-14T21:38:37Z

wp-modules/app/js/src/hooks/useForceRerend.ts

+		// Do not cause a re-render on first render.
+		if ( initialRender.current ) {
+			initialRender.current = false;
+		} else {
+			setForceUpdate( dependencies );
+		}


I'd vote to keep this if/else only if it adds a big performance boost.

Otherwise, simply calling setForceUpdate( dependencies ); might be fine.

kienstra · 2023-03-14T21:39:07Z

wp-modules/app/js/src/hooks/useForceRerend.ts

Sounds good, it's always good to look back and improve things

wp-modules/app/js/src/components/Patterns/PatternGrid.tsx

kienstra · 2023-03-15T02:05:06Z

wp-modules/app/js/src/hooks/useForceRerender.ts

+/**
+ * Re-render the calling component when the window is resized.
+ *
+ * Optionally include an array of dependencies to trigger a re-render.


Suggested change

* Optionally include an array of dependencies to trigger a re-render.

* Include an array of dependencies to trigger a re-render.

One last nitpick 🤣

I just noticed that and fixed right before I saw this 😆

Covered with 0c1d8d9 — I went with: Re-render the calling component when the window is resized or dependencies update.

Love those 1-liner blocks:

Haha, you beat me to it!

That's a great 1-liner!

…into try/force-rerender-all-stale-previews

mike-day added 5 commits March 14, 2023 14:57

Create new useForceUpdate hook

1ea37f7

Use the new hook in PatternGrid

3f8679a

Delete useWindowResize

7054173

Change the hook name to useForceRerend

c6bec03

Linting

9fc3d65

mike-day commented Mar 14, 2023

View reviewed changes

mike-day requested review from kienstra, dreamwhisper and johnstonphilip March 14, 2023 20:36

kienstra approved these changes Mar 14, 2023

View reviewed changes

kienstra reviewed Mar 14, 2023

View reviewed changes

wp-modules/app/js/src/components/Patterns/PatternGrid.tsx Outdated Show resolved Hide resolved

mike-day added 2 commits March 14, 2023 20:12

Suggestions from code review

0884c93

Linting

98ba40d

kienstra reviewed Mar 15, 2023

View reviewed changes

mike-day added 2 commits March 14, 2023 21:05

Update doc block since param is no longer optional

0c1d8d9

Merge branch 'main' of https://github.com/studiopress/pattern-manager …

7618669

…into try/force-rerender-all-stale-previews

mike-day merged commit 5f5efb7 into main Mar 15, 2023

mike-day deleted the try/force-rerender-all-stale-previews branch March 15, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix preview width when searching or filtering patterns #115

Fix preview width when searching or filtering patterns #115

mike-day commented Mar 14, 2023 •

edited

Loading

mike-day Mar 14, 2023

kienstra Mar 14, 2023

mike-day Mar 14, 2023

kienstra Mar 14, 2023

mike-day Mar 14, 2023

kienstra Mar 14, 2023 •

edited

Loading

mike-day Mar 14, 2023

kienstra Mar 14, 2023

kienstra Mar 14, 2023 •

edited

Loading

mike-day Mar 15, 2023 •

edited

Loading

kienstra Mar 15, 2023

kienstra left a comment

kienstra commented Mar 14, 2023

kienstra left a comment

kienstra Mar 14, 2023

kienstra Mar 14, 2023 •

edited

Loading

kienstra Mar 14, 2023 •

edited

Loading

kienstra Mar 15, 2023

kienstra Mar 15, 2023

mike-day Mar 15, 2023

mike-day Mar 15, 2023 •

edited

Loading

kienstra Mar 15, 2023

kienstra Mar 15, 2023

	* Optionally include an array of dependencies to trigger a re-render.
	* Include an array of dependencies to trigger a re-render.

Fix preview width when searching or filtering patterns #115

Fix preview width when searching or filtering patterns #115

Conversation

mike-day commented Mar 14, 2023 • edited Loading

How to test

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

mike-day Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra left a comment

Choose a reason for hiding this comment

kienstra commented Mar 14, 2023

kienstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

kienstra Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-day Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-day commented Mar 14, 2023 •

edited

Loading

kienstra Mar 14, 2023 •

edited

Loading

kienstra Mar 14, 2023 •

edited

Loading

mike-day Mar 15, 2023 •

edited

Loading

kienstra Mar 14, 2023 •

edited

Loading

kienstra Mar 14, 2023 •

edited

Loading

mike-day Mar 15, 2023 •

edited

Loading