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(Pagination): automate pagination and overflow logic #3182

Merged
merged 19 commits into from Dec 5, 2019

Conversation

@evwilkin
Copy link
Contributor

evwilkin commented Oct 22, 2019

What:
fixes #3136

This feature allows the component to automatically paginate the users's data for them, and provides a "defaultToFullPage" flag that will automatically move the user back to the last full page of results when they change the number of rows to view per page.

Additional issues:

Copy link
Contributor

tlabaj left a comment

Hi. Can you update the title of the PR to be more descriptive please. Thanks!

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 22, 2019

PatternFly-React preview: https://patternfly-react-pr-3182.surge.sh

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 22, 2019

Codecov Report

Merging #3182 into master will decrease coverage by <.01%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3182      +/-   ##
==========================================
- Coverage   67.52%   67.52%   -0.01%     
==========================================
  Files         896      896              
  Lines       25112    25133      +21     
  Branches     2173     2178       +5     
==========================================
+ Hits        16958    16971      +13     
- Misses       7141     7147       +6     
- Partials     1013     1015       +2
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.28% <ø> (ø) ⬆️
#patternfly4 65.01% <79.31%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...eact-core/src/components/Pagination/Navigation.tsx 86.81% <100%> (+0.93%) ⬆️
...eact-core/src/components/Pagination/Pagination.tsx 81.25% <50%> (-1.29%) ⬇️
...rc/components/Pagination/PaginationOptionsMenu.tsx 87.03% <64.28%> (-8.21%) ⬇️
...nfly-react/src/components/Pagination/Pagination.js 27.27% <0%> (-0.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b110a3f...9e2a445. Read the comment docs.

@tlabaj tlabaj requested review from dlabrecq and seanforyou23 Oct 22, 2019
@evwilkin evwilkin changed the title Feat 3136 Feat(Pagination): automate pagination and overflow logic Oct 22, 2019
@evwilkin evwilkin changed the title Feat(Pagination): automate pagination and overflow logic feat(Pagination): automate pagination and overflow logic Oct 22, 2019
Copy link
Member

dlabrecq left a comment

How are these utils used with the pagination component?

I'm also curious how does this solve #1567 (e.g., using an offset). There is no example showing usage?

Ideally, the pagination component would be able to figure out what page you're on based on the total available rows and number of items per page.

Copy link
Member

seanforyou23 left a comment

I left a few questions/comments, looking good. On first pass, it seems to improve the intro-table tutorial quite a bit - excited to get this in!

@evwilkin evwilkin force-pushed the evwilkin:feat_3136 branch from 340a4fb to ad24d29 Nov 11, 2019
@evwilkin evwilkin requested review from seanforyou23 and dlabrecq Nov 11, 2019
Copy link
Member

seanforyou23 left a comment

This is looking really great! Left a couple of questions, beyond that the only thing I think we wanted to mention is that I'm seeing the following error in the console:

Screen Shot 2019-11-27 at 10 25 52 AM

Even with that error, the demo works fine so probably something simple. Nice work!

): void {
if (event.keyCode === KEY_CODES.ENTER) {
const inputPage = Navigation.parseInteger(this.state.userInputPage, lastPage) as number;
onPageInput(event, Number.isNaN(inputPage) ? (page as number) : inputPage);
onSetPage(event, Number.isNaN(inputPage) ? (page as number) : inputPage);
this.handleNewPage(event, Number.isNaN(inputPage) ? (page as number) : inputPage);

This comment has been minimized.

Copy link
@seanforyou23

seanforyou23 Nov 27, 2019

Member

I wonder if there's a better way of handling that page here could be a string or a number. Could we instead cast it to a number in the place where it returns a string so we can avoid having to use as number type assertion in so many places? No worries if not, just curious in case it's an easy improvement.

@evwilkin evwilkin force-pushed the evwilkin:feat_3136 branch from ad24d29 to c64bc2d Nov 27, 2019
Copy link
Member

dlabrecq left a comment

LGTM. Nice job

@@ -69,6 +85,8 @@ export interface PaginationProps extends React.HTMLProps<HTMLDivElement> {
perPage?: number;
/** Select from options to number of items per page. */
perPageOptions?: PerPageOptions[];
/** Indicate whether to show last full page of results when user selects perPage value greater than remaining rows */
defaultToFullPage?: boolean;

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Nov 29, 2019

Member

Curious if defaultToFullPage is a PatternFly pattern in core? If not, lets double check this behavior with a designer.

This comment has been minimized.

Copy link
@evwilkin

evwilkin Dec 2, 2019

Author Contributor

Thanks for calling this out @dlabrecq , doing this now

Copy link
Member

seanforyou23 left a comment

LGTM, nice work!

@evwilkin evwilkin requested review from mcoker and mcarrano Dec 2, 2019
…tch new last page of results when page number is automatically updated
@evwilkin evwilkin dismissed stale reviews from seanforyou23 and dlabrecq via f312fe8 Dec 2, 2019
Copy link
Contributor

mcoker left a comment

😁👍

Copy link
Member

mcarrano left a comment

Looks great @evwilkin Thanks for adding the documentation.

Copy link
Member

seanforyou23 left a comment

Love it! Nice work @evwilkin!

@evwilkin evwilkin merged commit af19ff8 into patternfly:master Dec 5, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@evwilkin evwilkin deleted the evwilkin:feat_3136 branch Dec 5, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Dec 5, 2019

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@1.1.44
  • @patternfly/react-core@3.126.0
  • @patternfly/react-docs@4.16.51
  • @patternfly/react-inline-edit-extension@2.14.2
  • demo-app-ts@3.14.0
  • @patternfly/react-integration@3.14.0
  • @patternfly/react-table@2.24.46
  • @patternfly/react-topology@2.11.32
  • @patternfly/react-virtualized-extension@1.3.45

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.