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

ERM-3220 Update pagination mechanisms for MCLs to work without stats #680

Merged
merged 10 commits into from
May 17, 2024

Conversation

CalamityC
Copy link
Contributor

@CalamityC CalamityC commented Apr 30, 2024

  • create new hook for fetch with no stats
  • add hasNextPage prop to usePrevNextPagination

* create new hook for fetch with no stats
* add hasNextPage prop to usePrevNextPagination
* create new hook for fetch with no stats
* add hasNextPage prop to usePrevNextPagination
Copy link

github-actions bot commented May 8, 2024

Jest Unit Test Statistics

    1 files  ±0    37 suites  ±0   2m 55s ⏱️ -2s
263 tests ±0  263 ✔️ ±0  0 💤 ±0  0 ±0 
270 runs  ±0  270 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 84d8703. ± Comparison against base commit a19aa79.

♻️ This comment has been updated with latest results.

@CalamityC CalamityC marked this pull request as ready for review May 15, 2024 14:00
@CalamityC CalamityC requested a review from EthanFreestone May 15, 2024 14:01
Copy link
Contributor

@EthanFreestone EthanFreestone left a comment

Choose a reason for hiding this comment

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

Overall code looks really good. Happy with the overall idea, just one small tweak requested to the queryKey logic in useFetchCurrentAndNext.

I think it's probably out of scope for this piece of work, but it'd be kinda neat to not limit useCurrentAndNext to just fetching 2 pages. Can it be renamed to something like "useFetchMultiplePages"? For now the logic can remain the same, but with a comment explaining that it's named like this to allow for future expansion to n pages down the line

lib/hooks/useFetchCurrentAndNext.js Outdated Show resolved Hide resolved
Renamed hook to "useFetchMultiplePages", so it will be primed for any future logic to expand past just two pages. For now it will make use of the page passed in the params object

Also changed out keyArray for getQueryKey which allows finer control over the queryKey for the implementing code

refs ERM-3220
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
33.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@EthanFreestone EthanFreestone merged commit 8748eb8 into master May 17, 2024
4 of 5 checks passed
@EthanFreestone EthanFreestone deleted the erm-3220 branch May 17, 2024 15:56
EthanFreestone added a commit that referenced this pull request May 28, 2024
…680)

* ERM-3220 Update pagination mechanisms for MCLs to work without stats

* add pageCount property

* ERM-3220 Update pagination mechanisms for MCLs to work without stats

* create new hook for fetch with no stats
* add hasNextPage prop to usePrevNextPagination

* ERM-3220 Update pagination mechanisms for MCLs to work without stats

* create new hook for fetch with no stats
* add hasNextPage prop to usePrevNextPagination

* refactor: Changed hasNextPage functionality

Now the hasNextPage overwrites the canGoNext calculation if it is provided

* refactor: Spread params tidy up

* ERM-3220 Update pagination mechanisms for MCLs to work without stats

* rename hook
* set defaukt page

* ERM-3220 Update pagination mechanisms for MCLs to work without stats

* remove comments
* add resetPageStore function

* ERM-3220 Update pagination mechanisms for MCLs to work without stats

* remove resetPageStore function

* feat: useFetchMultiplePages

Renamed hook to "useFetchMultiplePages", so it will be primed for any future logic to expand past just two pages. For now it will make use of the page passed in the params object

Also changed out keyArray for getQueryKey which allows finer control over the queryKey for the implementing code

refs ERM-3220

---------

Co-authored-by: Jack Golding <[email protected]>
Co-authored-by: Ethan Freestone <[email protected]>
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