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/search page progress bar #421

Merged
merged 3 commits into from
Mar 5, 2025
Merged

Conversation

marcellmueller
Copy link
Contributor

@marcellmueller marcellmueller commented Mar 4, 2025

#304

To test this set throttling to 3g in your network tab

I'm wondering if we should use react query isLoading instead of isFetching to prevent this from flashing after the initial page load.

Screenshot 2025-03-04 at 1 41 52 PM

{isFetching ? (
<ProgressBar animated now={100} className="mb-4" />
) : (
data?.pages?.map((pageData: PaginatedRecreationResourceDto) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit): This returns an array of arrays because you're mapping over data?.pages first, then again inside for pageData.data. This could cause some rendering issues. I believe it would be better to use flatMap i.e.

{isFetching ? (
  <ProgressBar animated now={100} className="mb-4" />
) : (
  data?.pages
    .flatMap((pageData: PaginatedRecreationResourceDto) =>
      pageData.data.map(
        (recreationResource: RecreationResourceDto) => (
          <RecResourceCard
            key={recreationResource.rec_resource_id}
            recreationResource={recreationResource}
          />
        )
      )
    )
)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout, I think that is from the react query refactor. I've changed this in my filter pills pr, though instead of mapping two arrays I am just doing this and mapping it once:

const searchResults = data?.pages?.[0].data ?? [];

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay great! LGTM than 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh just to keep you in the loop I misunderstood how it works with the new pagination, it does return array of pages I have added the flatmap in my new pr as you suggested thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hamidhabib-buttons just for my knowledge - could you elaborate on which rendering issues will the 2d array mapping cause?

@hamidhabib-buttons
Copy link
Contributor

Note: isFetching is the correct choice here since we want to show the bar every time the data is fetched.

@marcellmueller marcellmueller force-pushed the feat/search-page-progress-bar branch from ef4ed87 to ac80bfe Compare March 5, 2025 19:53
@marcellmueller marcellmueller merged commit 1747374 into main Mar 5, 2025
32 checks passed
@marcellmueller marcellmueller deleted the feat/search-page-progress-bar branch March 5, 2025 20:02
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