-
Notifications
You must be signed in to change notification settings - Fork 3
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
v5 component refactor: Pagination #245
Comments
Proposed Solution Design - Pagination Component StructurePagination is better suited for use with the Provider pattern. This pattern requires cloning the state to synchronize details across components, and to ensure the composed component should be used within a correct parent. Here is a simple POC for Pagination using the Provider pattern 1. Pagination
2. Pagination.BackwardButton
3. Pagination.ForwardButton
4. Pagination.PageIndicator
ExampleUsage with Static PageIndicator<Pagination initiatePage={1} totalPages={10}>
<Pagination.BackwardButton
onClick={(page) => handleOnBackButton(page)}
/>
<Pagination.PageIndicator/>
<Pagination.ForwardButton
onClick={(page) => handleOnForwardButton(page)}
/>
</Pagination> Usage with Dynamic PageIndicator<Pagination initiatePage={1} totalPages={10}>
<Pagination.BackwardButton
onClick={(page) => handleOnBackButton(page)}
/>
<Pagination.PageIndicator>
{({ currentPage }) => `Page ${currentPage}`}
</Pagination.PageIndicator>
<Pagination.ForwardButton
onClick={(page) => handleOnForwardButton(page)}
/>
</Pagination> Preview2024-12-30_09-45-28.mp4 |
@ss-dimasm thanks for the considerable effort on this solution design. I can see you've gone to a lot of thought putting this together and communicating the "why" behind each of the components you're proposing. 👏 While I appreciate the level of flexibility you've tried to bake into this, I think the proposal is currently over-engineered.
Given these two comments, I would suggestion the following simplification:
|
thanks, @kurtdoherty. yes my initial assumption was that pagination would be isolated within the component, and consumers could use it as they wanted (like the POC) However, considering your concern that "different products may take different approaches" is also valid, and it seems that simplification for Pagination would make it more beneficial for this case, especially since the Elements would then be responsible for the UI representation instead of managing state within a component Talking about the compositional approach, it may not needed when we only want the Pagination to deliver the UI presentational; might be better to put it within the component itself |
Hey @kurtdoherty , I've made an update on this, still in the same POC link as per above but I'll highlight the change below So, generally from a user's perspective, if we do this, it would make migration to the v5 elements simpler, since it's nearly "have the same API" as before; but IMO in terms of code I think we could put some in the atoms (i.e. PaginationForwardButton) they have their own responsibility while displaying the user interface example usage <Pagination
currentPage={page}
pageCount={TOTAL_PAGES}
onPageChange={setPage}
/> |
@ss-dimasm I don't have any strong opinions on whether you have intermediary components for the next page and previous page buttons, just so long as they're not exported for use (personally, I don't think they're needed, but 🤷). For now, I think we would just want the styled components ( |
it's fair @kurtdoherty , I'm assuming we've found the solution for pagination, and let's see how it will be constructed with production grade 🤔 |
@ss-dimasm Please note the updates Andrei has made to the design spec. They no longer have custom "Navigation button" components, rather they're using the standard Button component. This means we can do the same. |
that would be handy, I'll take a note of that 👍 @kurtdoherty |
As part of the v5 Elements release, each component will be reviewed and refactored to ensure best practice and design system alignment
Specification
Developer Checklist
Styles Only
andReact
component structuresRelease Checklist
The text was updated successfully, but these errors were encountered: