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

Implement Search Pagination #297

Merged
merged 29 commits into from
Feb 13, 2024
Merged

Implement Search Pagination #297

merged 29 commits into from
Feb 13, 2024

Conversation

aahei
Copy link
Member

@aahei aahei commented May 12, 2023

Implement Search Pagination

Description

  • Create a SearchPagination component
  • Remove the search result limit on WebSoc fuzzy search (may lower the performance)
    In addition:
  • Create stc/helpers/constants.ts to store PAGE_SIZE
  • Add the "No results found" prompt.

The current config is:

  • fetch all results at a time
  • display 10 records on a page

Screenshots

image

Final Checks:

  • Verify successful deployment
  • Delete branch

(optional)

  • Write tests
  • Write documentation

@aahei aahei linked an issue May 12, 2023 that may be closed by this pull request
3 tasks
@aahei aahei self-assigned this May 12, 2023
@aahei aahei linked an issue May 26, 2023 that may be closed by this pull request
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

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

LGTM! Great work 🔥

@js0mmer
Copy link
Member

js0mmer commented May 31, 2023

Also, I'm guessing the issue you opened for performance improvements is meant for a new task/PR. I'm going to go ahead and unlink it from this PR so it doesn't close when this one merges.

@aahei
Copy link
Member Author

aahei commented Jun 3, 2023

Just an inaccurate test:
Before improvement, it takes around 4000-5000 ms to load the course results with an empty query (~5900 results).
After improvement, it takes 2000-3000 ms.

@aahei
Copy link
Member Author

aahei commented Jun 9, 2023

Improvement: load only the first 5 pages initially, but when the user jumps to the 3rd page or after, load all pages.

@aahei
Copy link
Member Author

aahei commented Jun 9, 2023

Peterportal

Improvement: load only the first 5 pages initially, but when the user jumps to the 3rd page or after, load all pages.

This is a temporary solution. Currently, PPAPI does not support range search, see #310. We will change it to the actual range search upon the PPAPI update release.

Copy link
Member

@ethanwong16 ethanwong16 left a comment

Choose a reason for hiding this comment

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

LGTM! Just requested some minor changes :)) thanks!

site/src/component/SearchModule/SearchModule.tsx Outdated Show resolved Hide resolved
site/src/helpers/constants.ts Outdated Show resolved Hide resolved
@aahei
Copy link
Member Author

aahei commented Oct 21, 2023

Next todo: merge to Master

@aahei
Copy link
Member Author

aahei commented Nov 30, 2023

Next todo: test, also test #366

@aahei aahei requested a review from MFYLM November 30, 2023 07:21
Copy link
Member

@js0mmer js0mmer left a comment

Choose a reason for hiding this comment

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

Optimization seems to have broken for going past page 3. Gets stuck in an infinite loop. Dark mode theming needed for pagination component.

@js0mmer js0mmer self-requested a review February 1, 2024 06:27
@aahei
Copy link
Member Author

aahei commented Feb 1, 2024

Optimization seems to have broken for going past page 3. Gets stuck in an infinite loop. Dark mode theming needed for pagination component.

Thank you for noting this. It didn't happen when I tested it, so it might be some side effect when merging with the new code since this functionality was written a long time ago. I will try to investigate this on this weekend.

@js0mmer
Copy link
Member

js0mmer commented Feb 11, 2024

Optimization seems to have broken for going past page 3. Gets stuck in an infinite loop. Dark mode theming needed for pagination component.

Thank you for noting this. It didn't happen when I tested it, so it might be some side effect when merging with the new code since this functionality was written a long time ago. I will try to investigate this on this weekend.

Honestly, I think we can leave out the first 5 pages limiter. I've tested locally and performance seems to be fine with this current version of fuzzy search. I think we should also lower the search delay from 500ms to 300ms so response times are a bit faster.

@js0mmer
Copy link
Member

js0mmer commented Feb 12, 2024

Optimization seems to have broken for going past page 3. Gets stuck in an infinite loop. Dark mode theming needed for pagination component.

Thank you for noting this. It didn't happen when I tested it, so it might be some side effect when merging with the new code since this functionality was written a long time ago. I will try to investigate this on this weekend.

Honestly, I think we can leave out the first 5 pages limiter. I've tested locally and performance seems to be fine with this current version of fuzzy search. I think we should also lower the search delay from 500ms to 300ms so response times are a bit faster.

Nevermind, seems like it is necessary given that changing between catalogue/roadmap tabs slows down when searching the additional results.

@js0mmer
Copy link
Member

js0mmer commented Feb 13, 2024

Found a solution: moving the states for hasFullResults and lastQuery into the redux slice seems to prevent these infinite loops. Will implement along with theming then this should be good to go.

commit d6ca086
Author: Jacob Sommer <[email protected]>
Date:   Mon Feb 12 02:32:57 2024 -0800

    Add no results image

commit f3e9823
Author: Jacob Sommer <[email protected]>
Date:   Mon Feb 12 01:45:48 2024 -0800

    Refactor search pagination/hit container to simplify store usage

commit 58a73c5
Author: Jacob Sommer <[email protected]>
Date:   Mon Feb 12 01:32:40 2024 -0800

    Dark theme for pagination
Copy link

Deployed staging instance to https://staging-297.peterportal.org

@js0mmer js0mmer merged commit 4b34718 into master Feb 13, 2024
3 checks passed
@js0mmer js0mmer deleted the search-pagination branch February 13, 2024 05:34
@js0mmer js0mmer linked an issue Feb 13, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants