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

feat(recommend): add status state (loading, stalled, idle) #43

Merged
merged 2 commits into from
Jun 29, 2021

Conversation

francoischalifour
Copy link
Member

The problem

There was an issue where we rendered the fallbackComponent during first render because we checked props.recommendations.length === 0, which is the case before sending the request. Thus, when using RelatedProducts in fallbackComponent, it sent another request, which was unnecessary.

This solution

This PR adds the loading status of the component in the API, and uses it to render the fallback component when there's no recommendations and that the status is idle.

Possible statuses are idle, loading and stalled. The stalled status is useful to render product placeholders after xms, to reduce UI flashes as opposed to rendering it when loading.

In the future, we might want to expose the stalled threshold value to the user, so that they can synchronize their placeholder content with other pieces on their website.

Considerations

Notice that the useStatus hook in Preact and in React have an identical implementation, which is a bummer. I don't think we can use the same code here without additional tooling.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

Code looks good, but I think we could have a useStatus factory to share between react & preact

@@ -0,0 +1,23 @@
import { RecommendStatus } from '@algolia/recommend-vdom';
import { useEffect, useRef, useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make a factory where useEffect, useRef, useState as passed to the hook creator, so we can share the implementation?

Copy link
Member

Choose a reason for hiding this comment

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

Where would you see this implementation? If we have this duplication more frequently in the future, we could have a recommend-hooks package but right now it doesn't fit in VDOM or core IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

it could fit in vdom, seeing as only react-like api lives there at the moment. Whenever we would add Vue, I guess a different api than purely hooks would be needed, but not yet atm

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's a bit too soon to abstract this, but we should definitely define a strategy if these duplications need to grow.

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Looks clean!

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

let's keep it in mind for the future

@francoischalifour francoischalifour merged commit 9f6ef30 into next Jun 29, 2021
@francoischalifour francoischalifour deleted the feat/status-loading branch June 29, 2021 08:01
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.

3 participants