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

Jump to bottom

Add autoHide to Pagination #8615

Merged
merged 1 commit into from Dec 29, 2017
Merged

Add autoHide to Pagination #8615

merged 1 commit into from Dec 29, 2017

Conversation

camsong
Copy link
Contributor

@camsong camsong commented Dec 14, 2017 •
edited

It's quite cumbersome to show pagination when there is only 1 page.

Code like this can help but you need to define a const pageSize and not elegant.

const pageSize = 20;

<Table pagination={data.length > pageSize && { pageSize }} />

This is a common requirement so better to have a prop specifically for it.

autoHidePagination default is false, so it will not break any current behavior.

May refer to #4173

@camsong camsong force-pushed the table-autoHidePagination branch 2 times, most recently from 78564f0 to 992ebb1 Compare December 14, 2017 09:47
@yesmeck
Copy link
Member

yesmeck commented Dec 14, 2017 •
edited

Hmm, how about moving this prop to pagination <Table pagination={{ autoHide: true }} />?

@camsong
Copy link
Contributor Author

camsong commented Dec 14, 2017

@yesmeck sure, will do

yesmeck
yesmeck reviewed Dec 14, 2017
View reviewed changes
components/table/__tests__/Table.pagination.test.js Outdated
@@ -37,6 +37,21 @@ describe('Table.pagination', () => {
expect(wrapper).toMatchSnapshot();
});

it.only('should have pager when change pagination from false to undefined', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Remove "only".

@yesmeck
Copy link
Member

yesmeck commented Dec 14, 2017

And, you should PR to master.

@codecov
Copy link

codecov bot commented Dec 14, 2017 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@5cda605). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #8615   +/-   ##
========================================
  Coverage          ?     85%           
========================================
  Files             ?     227           
  Lines             ?    4676           
  Branches          ?    1369           
========================================
  Hits              ?    3975           
  Misses            ?     701           
  Partials          ?       0
Impacted Files Coverage Δ
components/pagination/Pagination.tsx 94.11% <ø> (ø)

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 5cda605...25a0210. Read the comment docs.

@camsong camsong changed the title Add autoHidePagination to Table Add autoHide to Pagination Dec 14, 2017
@camsong
Copy link
Contributor Author

camsong commented Dec 14, 2017 •
edited

@yesmeck I'm still using 2.x, I will make a PR to master as well.

@yesmeck
Copy link
Member

yesmeck commented Dec 14, 2017 •
edited

You still need to PR to master, If we need release 2.x, we will cherry pick this feature to 2.x-stable branch.

@camsong camsong changed the base branch from 2.x-stable to master December 14, 2017 14:58
@camsong camsong force-pushed the table-autoHidePagination branch 3 times, most recently from ddfdd3f to 930bdf4 Compare December 14, 2017 15:18
yesmeck
yesmeck reviewed Dec 15, 2017
View reviewed changes
components/pagination/Pagination.tsx Outdated
@@ -50,6 +51,10 @@ export default class Pagination extends React.Component<PaginationProps, {}> {
}

render() {
// When autoHide is true and there is only 1 page, hide the pager
if (this.props.autoHide === true && (this.props.total || 0) <= (this.props.pageSize || 10)) {
Copy link
Member

Choose a reason for hiding this comment

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

The defaultPageSize comes from rc-pagination, I think rc-paginaion is a better place a add this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, done

@afc163
Copy link
Member

afc163 commented Dec 15, 2017 •
edited

autoHide => hideOnSinglePage

@camsong camsong mentioned this pull request Dec 15, 2017
Merged
@camsong
Copy link
Contributor Author

camsong commented Dec 15, 2017

@yesmeck @afc163 Done, tests should pass after rc-pagination released.

@yutingzhao1991
Copy link
Contributor

@camsong rc-pagination published, you need to upgrade it to ~1.13.0 in antd.

@camsong
Copy link
Contributor Author

camsong commented Dec 18, 2017 •
edited

@yutingzhao1991 Thanks so much.

It seems antd also need to be published to get TypeScript support 😏
Right now we are using 2.x version with TS, can you help to make a release?

@yesmeck
Copy link
Member

yesmeck commented Dec 18, 2017 •
edited

接口先自己覆盖下吧typings/index.d.ts。分支之前说错了，需要 PR 到 feature-3.1。

/// <reference types="react" />
import * as React from 'react';

declare module 'antd/lib/pagination/Pagination' {
  interface PaginationProps {
    hideOnSinglePage?: boolean;
  }
}

@camsong camsong changed the base branch from master to feature-3.1 December 18, 2017 03:23
@camsong camsong force-pushed the table-autoHidePagination branch 2 times, most recently from b59610b to 57d0a60 Compare December 18, 2017 08:21
@camsong
Copy link
Contributor Author

camsong commented Dec 18, 2017

@yesmeck target branch changed, your patch works for now.

@yesmeck
Copy link
Member

yesmeck commented Dec 18, 2017

Should update rc-pagination to 1.13.0.

@yesmeck yesmeck mentioned this pull request Dec 29, 2017
Merged
@yutingzhao1991 yutingzhao1991 changed the base branch from feature-3.1 to master December 29, 2017 12:14
@yutingzhao1991 yutingzhao1991 merged commit 1af1534 into ant-design:master Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yesmeck yesmeck yesmeck left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@camsong @yesmeck @afc163 @yutingzhao1991