Skip to content
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

feat: implement externally controlled mode for TablePagination [WD-8505] #1034

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

mas-who
Copy link
Contributor

@mas-who mas-who commented Jan 31, 2024

Done

  • Introduced externally controlled mode for the TablePagination component
  • added test for externally controlled mode for the TablePagination component. You can refer to the test for example usage in externally controlled mode.

QA

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

  • Check in projects that uses TablePagination if server pagination works.

@webteam-app
Copy link

Demo starting at https://react-components-1034.demos.haus

@mas-who mas-who changed the title feat: implement server pagination mode for TablePagination feat: implement server pagination mode for TablePagination [WD-8505] Jan 31, 2024
@mas-who mas-who force-pushed the implement-server-pagination branch 2 times, most recently from 8b5575b to 1c57c2b Compare January 31, 2024 16:42
@mas-who mas-who force-pushed the implement-server-pagination branch from 1c57c2b to bbadd47 Compare February 1, 2024 09:54
@mas-who
Copy link
Contributor Author

mas-who commented Feb 1, 2024

Decided to move the presentation part of the component to TablePaginationControls, then inside TablePagination we will adjust component behaviour depending on the externallyControlled prop. It is okay to keep usePagination in TablePagination actually since we just pass it with no data if externallyControlled is true. I think having a completely separate component for un-controlled pagination will make things a little messy and result in duplicated code.

The cool thing is that the test I wrote previously for server-pagination didn't have to change at all, so it served as a bit of a setup for TDD 🙂

@mas-who mas-who changed the title feat: implement server pagination mode for TablePagination [WD-8505] feat: implement externally controlled mode for TablePagination [WD-8505] Feb 1, 2024
@mas-who mas-who force-pushed the implement-server-pagination branch from bbadd47 to 1dc2235 Compare February 1, 2024 10:05
Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for applying the new structure, I think this is way better than before!

Two tiny suggestions for further improvements below.

src/components/TablePagination/TablePagination.tsx Outdated Show resolved Hide resolved
src/components/TablePagination/TablePagination.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the implement-server-pagination branch from 1dc2235 to 78f88d7 Compare February 2, 2024 16:15
Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for making this nice :)
Just one more idea to a bit more defensive implementation, feel free to address this or not.

@mas-who mas-who merged commit 8fc31de into canonical:main Feb 5, 2024
6 checks passed
Copy link

github-actions bot commented Feb 5, 2024

🎉 This PR is included in version 0.50.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants