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

Fix only first page being loaded in PaginatedMultiSelect #6978

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jan 31, 2025

Part of https://github.com/hypothesis/support/issues/178
Depends on hypothesis/frontend-shared#1864

Use the fixes introduced in hypothesis/frontend-shared#1864 to ensure all pages are loaded in PaginatedMultiSelect as you scroll down.

Test steps

  1. Go to https://hypothesis.instructure.com/courses/319/assignments and log in as instructor
  2. Launch a "bunch" of localhost assignments from there, to build a relatively long list.
  3. Go to http://localhost:8001/dashboard/courses/1. You should see all assignments you lunched listed there, and they should also show in the "Assignments" dropdown.
  4. Apply this diff, which does two things: a) reduce the items per page returned by paginated API calls and b) reduce the size of Select popovers, so that scroll appears even with very few items on it.
    diff --git a/lms/static/scripts/frontend_apps/components/dashboard/PaginatedMultiSelect.tsx b/lms/static/scripts/frontend_apps/components/dashboard/PaginatedMultiSelect.tsx
    index 384219a3a..f5613e0b0 100644
    --- a/lms/static/scripts/frontend_apps/components/dashboard/PaginatedMultiSelect.tsx
    +++ b/lms/static/scripts/frontend_apps/components/dashboard/PaginatedMultiSelect.tsx
    @@ -98,6 +98,7 @@ export default function PaginatedMultiSelect<TResult, TSelect>({
           onChange={onChange}
           aria-label={`Select ${entity}`}
           containerClasses="!w-auto min-w-44"
    +      popoverClasses="!max-h-24"
           buttonContent={buttonContent}
           data-testid={`${entity}-select`}
           onPopoverScroll={e => {
    diff --git a/lms/views/dashboard/pagination.py b/lms/views/dashboard/pagination.py
    index 38325e270..a865d88f8 100644
    --- a/lms/views/dashboard/pagination.py
    +++ b/lms/views/dashboard/pagination.py
    @@ -15,7 +15,7 @@ LOG = logging.getLogger(__name__)
     T = TypeVar("T")
     
     
    -MAX_ITEMS_PER_PAGE = 100
    +MAX_ITEMS_PER_PAGE = 3
     """Maximum number of items to return in paginated endpoints"""
  5. Open the assignments dropdown, which should only show three assignments. Scroll down and observe how new pages continue loading until all assignments are there.

In main, if you apply the same diff, you'll see how only the first page of assignments is loaded even if you scroll.

Warning

There's an unrelated issue with students when loaded from the the roster, where we seem to be returning the first page on every request, even when a cursor is provided.
This PR does not cover fixing that.

@robertknight
Copy link
Member

Use the fixes introduced in hypothesis/frontend-shared#1864 to ensure all pages are loaded in PaginatedMultiSelect as you scroll down.

This PR just avoids the use of a deprecated API right? Any code that still uses the old prop will still get the fix.

@acelaya
Copy link
Contributor Author

acelaya commented Jan 31, 2025

Use the fixes introduced in hypothesis/frontend-shared#1864 to ensure all pages are loaded in PaginatedMultiSelect as you scroll down.

This PR just avoids the use of a deprecated API right? Any code that still uses the old prop will still get the fix.

This PR is still missing the actual fix, which is updating to the latest version of frontend-shared.

I also took the opportunity to stop using the deprecated prop, but the fix would take effect even without that change.

@acelaya acelaya requested a review from robertknight January 31, 2025 11:22
@acelaya acelaya marked this pull request as ready for review January 31, 2025 11:22
@acelaya acelaya merged commit fdd56e9 into main Jan 31, 2025
9 checks passed
@acelaya acelaya deleted the scroll-event-fix branch January 31, 2025 11:24
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.

2 participants