hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
46 stars 14 forks source link

Return pageSize and hasMorePages as part of usePaginatedAPIFetch result #6845

Closed acelaya closed 2 weeks ago

acelaya commented 2 weeks ago

This PR updates the result returned by usePaginatedAPIFetch, so that two new properties are included:

These are useful for https://github.com/hypothesis/product-backlog/issues/1570, as they will allow us to display an exact number of items in the dropdown, or something like 100+ where 100 is the page size, but allowing to change the page sizes for every individual dropdown, without having to change the logic to represent the count badge.

Todo

acelaya commented 2 weeks ago

I wonder whether we want to return the page size or the slightly simpler fact of whether there are more pages or not. The latter is technically correct and I think would be sufficient for indicating in a badge whether the count is exact or a lower bound. There would be a slight change to user-facing behavior in that this lower bound could update if the user started scrolling the list.

The product desire at the moment is to display the lower bound if there are more pages, not to update the badge value as soon as new pages are loaded, so we need to know what is the page size.

If this decision is changed, we can still do as you suggest even with this approach, so I would be inclined to not simplify it to a boolean flag, as this is more flexible.

acelaya commented 2 weeks ago

I wonder whether we want to return the page size or the slightly simpler fact of whether there are more pages or not. The latter is technically correct and I think would be sufficient for indicating in a badge whether the count is exact or a lower bound. There would be a slight change to user-facing behavior in that this lower bound could update if the user started scrolling the list.

The product desire at the moment is to display the lower bound if there are more pages, not to update the badge value as soon as new pages are loaded, so we need to know what is the page size.

If this decision is changed, we can still do as you suggest even with this approach, so I would be inclined to not simplify it to a boolean flag, as this is more flexible.

@robertknight I have asked in the issue to clarify exactly what is the desired behavior, and then make the best decision https://github.com/hypothesis/product-backlog/issues/1570#issuecomment-2464254027

robertknight commented 2 weeks ago

If this decision is changed, we can still do as you suggest even with this approach, so I would be inclined to not simplify it to a boolean flag, as this is more flexible.

The current API gives different information than an allPagesFetched flag (complete?), but it isn't a substitute for all use cases. With the current proposed API, there is no way to tell if there are more pages left to fetch for example (unless I'm missing something?). The current downstream code just calls loadNextPage unconditionally when you scroll to the bottom of the list. With an allPagesFetched flag on the other hand you can tell if there is more data to come, but not how big the individual pages are.

I have asked in the issue to clarify exactly what is the desired behavior, and then make the best decision

OK, I'm going to approve this on the assumption that the page size does turn out to be useful information. I noted above that you might still need to expose the "all pages fetched" state for other reasons.

acelaya commented 2 weeks ago

I noted above that you might still need to expose the "all pages fetched" state for other reasons.

Yeah, you are right there.

I'm not going to merge this for now, until everything has been clarified from a product point of view.

acelaya commented 2 weeks ago

I have decided to include both pageSize?: number and hasMorePages: boolean, for the sake of unblocking the main task.

The logic is pretty simple anyway and this should allow to go in both directions.