Fix Spinner does not stop when favorites is an empty list

[renintw]

If a user didn't add any pattern to its favorite list, when switching to other categories, the loading spinner would keep spinning even if the loading is over.

Root cause

Details
This is the logic that decides whether to stop showing the spinner.

pattern-directory/public_html/wp-content/themes/pattern-directory/src/components/context-bar/index.js

Line 115 in 21e69c4

'context-bar__spinner--is-hidden': ! isLoadingPatterns,

Its logic comes from this function:

pattern-directory/public_html/wp-content/themes/pattern-directory/src/components/context-bar/index.js

Line 69 in 21e69c4

const isLoading = isLoadingPatternsByQuery( _query );

pattern-directory/public_html/wp-content/themes/pattern-directory/src/store/selectors.js

Lines 20 to 24 in 21e69c4

	export function isLoadingPatternsByQuery( state, query ) {
	const queryString = getQueryString( query );
	const page = query?.page || 1;
	return ! Array.isArray( state.patterns.queries?.[ queryString ]?.[ page ] );
	}

isLoading would always be true when the favorites list is empty (i.e., You won't see any patterns under Favorites All)
Once there's a pattern in the favorites list, isLoading would become false (explained later), and the problem is solved then.

The reason isLoading is always true in this case is that the queryString could not be found in state.patterns.queries.
Why so? That's because we didn't call getPatternsByQuery( modifiedQuery )

pattern-directory/public_html/wp-content/themes/pattern-directory/src/components/my-favorites/index.js

Line 37 in 21e69c4

const posts = favorites.length ? getPatternsByQuery( modifiedQuery ) : [];

when the fav list is empty, which causes that the queryString isn't stored via dispatching theloadPatterns.
And this is also why isLoading would become false once there's a pattern in the favorites list - the getPatternsByQuery( modifiedQuery ) is called.

pattern-directory/public_html/wp-content/themes/pattern-directory/src/store/resolvers.js

Lines 53 to 58 in 21e69c4

	yield loadPatterns( queryString, {
	page: query.page || 1,
	patterns: results,
	total: total,
	totalPages: totalPages,
	} );

So here comes the first solution

Sol 1
Call getPatternsByQuery( modifiedQuery ) no matter if the favorites.length===0 or not

const temp = getPatternsByQuery( modifiedQuery );
const posts = favorites.length ? temp : [];

However, though it solved the isLoading is always true problem, it would bring another issue.
Since we don't have any items in the favorites list for the moment, the include in modifiedQuery would be[], and when this modifiedQuery is sent as payload through the getPatternsByQuery( modifiedQuery ), the include=[] would be filtered out in advance in addQueryArgs before sending to the endpoint '/wp/v2/wporg-pattern'.

pattern-directory/public_html/wp-content/themes/pattern-directory/src/components/my-favorites/index.js

Line 35 in 21e69c4

const modifiedQuery = { ..._query, include: favorites };

pattern-directory/public_html/wp-content/themes/pattern-directory/src/store/resolvers.js

Lines 48 to 49 in 21e69c4

	const response = yield apiFetch( {
	path: addQueryArgs( '/wp/v2/wporg-pattern', { ...query, locale: JSON.parse( wporgLocale ) } ),

Usually, for a normal case, it would include something like include[0]=732 in the payload when sending to that endpoint and gets the response with the pattern data of patternId 732.
And since in the case mentioned above, include=[] is filtered out from payload, the endpoint response would still return with our suggested favorite patterns for a certain category and the patterns would be stored in state.patterns.queries.



Since there are favorite patterns returned by endpoint and stored into state, it would make the number of patterns in a category wrong.

pattern-directory/public_html/wp-content/themes/pattern-directory/src/components/context-bar/index.js

Line 74 in 21e69c4

count: getPatternTotalsByQuery( _query ),

pattern-directory/public_html/wp-content/themes/pattern-directory/src/store/selectors.js

Lines 60 to 63 in 21e69c4

	export function getPatternTotalsByQuery( state, query ) {
	const queryString = getQueryString( query );
	return state.patterns.queries?.[ queryString ]?.total || 0;
	}

This issue could be solved by making it still send include in payload with value like [-1] to make it return [], which is definitely a bad idea and confusing solution that needs some comments.

Or we can modify the logic of the endpoint and the addQueryArgs instead to make it send something legit when include=[] from client to the endpoint and have some corresponding process to it. And this is where I found it difficult for the moment to locate where and how the query string params pattern-categories and include are processed. Might need more studies and some clarifying for my doubts on my side if we choose this solution at the end of the day.

Sol 2
Add a condition check for query.include in isLoadingPatternByQuery.

It's not that ideal as it's kind of specifically for a certain case and might need some comments.
But it probably is safe for the moment to my knowledge as the include=[favorite patternIds] is only used and included by MyFavorites.

Sol 3
Pass isEmpty as isFavoritesEmpty to ContextBar
It's also not that ideal as far as I'm concerned as it's a bit not clean and consistent.
But the code could speak itself.

---

Fixes #362

Screencasts

Before:
https://d.pr/v/pTI4lI
After:
https://d.pr/v/pdkMqt

How to test the changes in this Pull Request:

1. Go to Favorites -> All
2. Empty the favorites list
3. Switch to other categories
4. Spinner should not be seen

[ryelle]

Thanks for the detailed write-up! I think the solution you landed on is the best balance of simplicity & functionality 🙂

I have one bit of feedback on the prop name, but this is working well in my testing 👍🏻

[StevenDufresne]

Well explained!

I'm a bit hesitant introducing favorite specific logic in the ContextBar seeing that it's used in contexts where favorites don't exist, at least in the same way. Additionally if we were to drop the favorite from the prop, it isn't apparent as to why loading should stop because something is empty.

Instead of passing the props down into the ContextBar is there a way to tell if all requests have resolved?

[renintw]

Well explained!

I'm a bit hesitant introducing favorite specific logic in the ContextBar seeing that it's used in contexts where favorites don't exist, at least in the same way. Additionally if we were to drop the favorite from the prop, it isn't apparent as to why loading should stop because something is empty.

Instead of passing the props down into the ContextBar is there a way to tell if all requests have resolved?

I'm thinking that isLoadingPatternsByQuery from the selector is kind of the way to tell if all requests have been resolved. It would work correctly if we revised the logic with the Sol 1 mentioned in the PR description except for the number of favorites patterns would go wrong as the include=[] is removed from the request and the endpoint doesn't response as expected.

About not passing props down to ContextBar to make it looks weird in it, I'm considering that maybe we could make ContextBar simpler and general by getting rid of useSelect inside it. Let its parent components pass down the data and decide its behavior.

Some perks of the change:

1. Some selectors will only be called one time.
2. The data is controlled and stored in the parent component. - know where to find the data and how it's retrieved.
3. We could still pass down props with the name like isFavoritesEmpty and the ContextBar would only consider it just as its required props say, isLoading. - the idea of favorite would not exist in ContextBar then.

Drawback:

1. Parent component would be messier.

[StevenDufresne]

I'm thinking that isLoadingPatternsByQuery from the selector is kind of the way to tell if all requests have been resolved. It would work correctly if we revised the logic with the Sol 1 mentioned in the PR description except for the number of favorites patterns would go wrong as the include=[] is removed from the request and the endpoint doesn't response as expected.

Yes, but I was also thinking something within apiFetch, does it track it somehow? Probably not.

About not passing props down to ContextBar to make it looks weird in it, I'm considering that maybe we could make ContextBar simpler and general by getting rid of useSelect inside it. Let its parent components pass down the data and decide its behavior.

Yep, that's sounds fine. If we do use a Parent component do we need to make any other changes? Will PatternGridMenu which is shared still work or will that need some refactoring?

[renintw]

Yep, that's sounds fine. If we do use a Parent component do we need to make any other changes? Will PatternGridMenu which is shared still work or will that need some refactoring?

I'm tempted to consider PatternGridMenu as a general and common component as well, and the changes would be required on both MyFavorites and Patterns where I think are the suitable places to fetch components' related data and then pass down. (i.e., All required favorite data is fetched in Myfavorites, and required pattern data is fetched in Pattern)