-
-
Notifications
You must be signed in to change notification settings - Fork 785
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
Add index/total count to end of pagination buttons #490
Conversation
const firstItemCount:number = Math.min((currentPage-1)*itemsPerPage+1, totalItems); | ||
const lastItemCount:number = Math.min(firstItemCount+(itemsPerPage-1), totalItems); | ||
const indexText:string = `${firstItemCount}-${lastItemCount}/${totalItems}`; |
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.
You don't actually need to specify the type since the type is implied from the initialization.
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.
I know, my C days just give me anxiety regarding inferred types.
<Button | ||
variant="secondary" | ||
disabled={true} | ||
> | ||
{indexText} | ||
</Button> |
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.
Not entirely sure how I feel about a disabled button. I honestly think it looks fine with just a <div>
, it strongly implies that it's not part of the actual navigation.
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.
Eh, I disagree, but more fundamentally -- I think an index is part of the navigation of a paginated structure. Yes, it's not a functional part, but it conveys key information about navigation to the user.
1135bba
to
d8dd7a4
Compare
I am inclined to agree with @InfiniteTF on the placement of the index/total count. I think we should probably be looking at other sites and apps for guidance on this one. PH places it at the top of the page, just before the results. This is probably the ideal in my opinion: Google just shows the page and the total, meh: Radarr just gives you the total: As it is, it just looks a little tacked-on: It also looks broken in mobile view: My preference would be a small section above the results, like the PH example. |
My only concern there is that I feel we don't really have any UI elements that are analogous to a header (in the sense of results-for-query-foobar). Putting it between the filter and the content might look a bit weird as a result. Maybe putting it between the content and pagination in ListHook? I'd be inclined to then just break it out into its own component to facilitate that, though. On the other hand, I was also thinkin' we could use it as a way to pop up a modal so users could skip to a particular range, and I don't know if it's a great idea to influence pagination outside pagination. Looking at the mobile view, and how it's implemented in Pagination.tsx -- I'm inclined to suggest we shorten the index string (dropping " of " in favor of "/") for mobile views, but I have to confess, I have no idea how that's achieved with these classNames on those first/last buttons. |
d8dd7a4
to
05812ef
Compare
- Only appears to happen at all in particularly odd circumstances, but still.
ed7169a
to
6900c79
Compare
- Something something text should be seen and not heard, har har
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.
Looks and tests ok.
Very simple, just chucks a fake button on the end of the pagination buttons. Looked a bit awkward by itself on the no-pagination case, so left it out there, but hopefully most folks know how to count up to 40.