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

MPDX-8515 - Fetching all pages on the coaches page #1258

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Jan 16, 2025

Description

As there was a limit on the number of coaches we could grab at one time from the GraphQL, I've implemented the useFetchAllPages functionality into the coaches page to ensure all coaches are rendered and not just the first 25.

This PR continues the work done on PR #1257 Where I adjusted the number of coaches to be fetched from 25 to 100. However, the server limits us to only fetch 50 at a time.

To Test, please impersonate Shelley and navigate to her coaches page. You can find Shelley's email address from this HelpScout ticket

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@dr-bizz dr-bizz added the Preview Environment Add this label to create an Amplify Preview label Jan 16, 2025
@dr-bizz dr-bizz requested a review from canac January 16, 2025 14:16
Copy link
Contributor

Copy link
Contributor

github-actions bot commented Jan 16, 2025

Bundle sizes [mpdx-react]

Compared against e7677b9

No significant changes found

@canac
Copy link
Contributor

canac commented Jan 16, 2025

@dr-bizz Since most users don't have many people they're coaching, what do you think about just using useFetchAllPages to load all of the coaches if there are multiple pages? Even people like Shelly with lots of coached staff, it's only 99, not hundreds. I think that would simplify our code, and could be a better experience for our users since infinite scroll has had some issues on other pages.

@dr-bizz dr-bizz force-pushed the remove-limit-from-fetching-coaches branch 2 times, most recently from 9181681 to bdbe0e4 Compare January 16, 2025 18:28
@dr-bizz
Copy link
Contributor Author

dr-bizz commented Jan 16, 2025

@dr-bizz Since most users don't have many people they're coaching, what do you think about just using useFetchAllPages to load all of the coaches if there are multiple pages? Even people like Shelly with lots of coached staff, it's only 99, not hundreds. I think that would simplify our code, and could be a better experience for our users since infinite scroll has had some issues on other pages.

@canac I've updated the query to use useFetchAllPages

Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

This looks really good! I've got a couple questions on it.

src/components/Coaching/CoachingList.tsx Outdated Show resolved Hide resolved
src/components/Coaching/CoachingList.tsx Outdated Show resolved Hide resolved
src/lib/apollo/cache.ts Outdated Show resolved Hide resolved
@dr-bizz dr-bizz requested a review from canac January 16, 2025 19:37
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

Apart from the flakiness with loading multiple pages concurrently, this looks great!

@dr-bizz dr-bizz changed the title MPDX-8515 - Adding infinite scroll to the coaches page MPDX-8515 - Fetching all pages on the coaches page Jan 17, 2025
@dr-bizz dr-bizz force-pushed the remove-limit-from-fetching-coaches branch from 521d763 to 8027851 Compare January 17, 2025 12:41
@dr-bizz dr-bizz merged commit 28bd6d3 into main Jan 17, 2025
19 checks passed
@dr-bizz dr-bizz deleted the remove-limit-from-fetching-coaches branch January 17, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants