-
Notifications
You must be signed in to change notification settings - Fork 27
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
Disabled nextPage button on Admin-Users list if no more results to show #8037
Disabled nextPage button on Admin-Users list if no more results to show #8037
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, this isn't going to cut it - the load for totals lags way behind, so there's a significant disconnect between the headline updates with the total number and the nav unlocking.
it also causes my browser to lock up (getting the "your browser has become unresponsive" message in FF), so the double-load might be a bit much.
suggesting to drop the offset/limit params on the API request, and then paginate on the entire result set each time. this might allow us to drop the @limitless
argument from the <PagedlistControls>
component.
I was confused by your comment initially, because I had only been testing viewing users with an input filter, and it never took very long. I just went to the page with no input filter and see what you mean -_- GraphQL attempt coming in next... |
8a58c26
to
c0ee84e
Compare
After realizing that GraphQL wasn't any faster to grab all users (20k+ results), I went with @stopfstedt's idea overfetching by 1 on the limit and comparing. That seems to work and doesn't require a second API call. It loses the Also, as an added change, I made it so that the grid of users shows a spinner when it's searching, because there's no visual feedback when you change the input filter or change the offset. Maybe this isn't the slickest implementation, but it's a start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fixes ilios/ilios#5544
My approach required adding another Ember Data query to be run to get the total users (as the main query limits the user count for the UI), but we get the benefit of disabling the nextPage button if appropriate AND a user count.
I understand if this is not ideal, so if it needs to be rejected (or maybe just modified?) I understand.