-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-1220 Search Results Part 1: Generic Pagination #231
DOP-1220 Search Results Part 1: Generic Pagination #231
Conversation
const canIncrementPage = useMemo(() => currentPage < totalPages, [currentPage, totalPages]); | ||
return ( | ||
<PaginationContainer> | ||
<PaginationButton |
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.
Any reason you went with this approach rather than LeafyGreen's IconButton?
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.
Ah, didn't even know that was a thing! I swapped over to it, thanks!
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.
Thanks for the quick fix! I'm looking at the staging link now and am not seeing the hover state for the icon buttons (demo here). Any idea why?
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.
Ah they are disabled
right now (mainly because there is no data so no pagination), but specifically:
- We are at page 1, so decrementing is disabled
- We are beyond the last page, so incrementing is disabled
When they are re-enabled once we have data, I'll double check for that hover state
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.
Just two non-blocking notes, looks great!
/* Below removes default hover effects from button */ | ||
background-image: none; | ||
border: none; | ||
box-shadow: none; | ||
:before { | ||
display: none; | ||
} | ||
:after { | ||
display: none; | ||
} |
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.
We probably don't need this anymore
|
||
const PaginationButton = styled(IconButton)` | ||
background-color: #fff; | ||
height: ${BUTTON_HEIGHT}; |
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.
[nit] Perhaps it would be better to use IconButton
's size
prop, but either way!
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.
Ah, well it turns out the default size is the same as this!
Thanks Sophie! |
Staging Guides
Staging Ecosystem
^ For these I just left the dropdown open for easier inspection. That is not a change made in this PR.
This PR adds a generic
Pagination
component to be used with search results. I also added interaction and snapshot testing for it. This component lets a parent component control the current page and enables/disables actions accordingly.