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(component): create basic pagination component #188

Merged
merged 12 commits into from
Sep 16, 2019
Merged

feat(component): create basic pagination component #188

merged 12 commits into from
Sep 16, 2019

Conversation

jordan-massingill
Copy link

@jordan-massingill jordan-massingill commented Sep 4, 2019

WHAT

Created Pagination component to be used with lists and tables

TESTING

jest testing

SCREENSHOTS

screencapture-localhost-3000-pagination-2019-09-12-13_58_29

Copy link
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

A couple more things:

  • Write some unit tests for your new feature.
  • You'll need to add a documentation page so you can show how to use your component as well as what props are being passed and how they are useful. See PAGES.md for the structure and layout of the page.

Copy link
Member

@deini deini left a comment

Choose a reason for hiding this comment

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

Can we add some screenshots to the PR description 🙏

@jordan-massingill jordan-massingill changed the title feat(component): create basic pagination component feat(component): create basic pagination component WIP Sep 5, 2019
Copy link
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

Minor comments, but looking really good!

packages/docs/components/SideNav/SideNav.tsx Outdated Show resolved Hide resolved
packages/docs/pages/Pagination/PaginationPage.tsx Outdated Show resolved Hide resolved
packages/big-design/src/components/Pagination/styled.tsx Outdated Show resolved Hide resolved
}

setItemRange([firstItemInRange, lastItemInRange]);
}, [currentPage, currentRange, totalItems]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but if totalItems changes, maxPages will not update. So with that in mind, you'll maybe want to have another useEffect listening to only totalItems and update it whenever that changes.

Copy link
Author

Choose a reason for hiding this comment

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

Since the state of currentPage is dependent on totalItems and currentRange props, it should/does change dynamically without the need for a useEffect listener (at least, that has been my experience with the component). We may need to look at messing around with this to be certain though.

@jordan-massingill jordan-massingill changed the title feat(component): create basic pagination component WIP feat(component): create basic pagination component Sep 11, 2019
@jordan-massingill jordan-massingill merged commit d79ede5 into bigcommerce:master Sep 16, 2019
@jordan-massingill jordan-massingill deleted the CHP-6021 branch September 16, 2019 16:26
amckemie pushed a commit to amckemie/big-design that referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants