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

Table column filter could be ReactNode #14777

Merged
merged 7 commits into from Feb 15, 2019
Merged

Conversation

vinhnph-ibl
Copy link
Contributor

First of all, thank you for your contribution! 😄

New feature please send pull request to feature branch, and rest to master branch.
Pull request will be merged after one of collaborators approve.
Please makes sure that these form are filled before submitting your pull request, thank you!

[中文版模板 / Chinese template]

This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Branch merge
  • Other (about what?)

What's the background?

  1. Describe the source of requirement.
    While using typescript, ColumnFilterItem text type of Table is valid only by string. It should be either string or ReactNode
  2. Resolve what problem.
    Filter item could be string or ReactNode
  3. Related issue link.

API Realization (Optional if not new feature)

  1. Basic thought of solution and other optional proposal.
  2. List final API realization and usage sample.
  3. GIF or snapshot should be provided if includes UI/interactive modification.

What's the effect? (Optional if not new feature)

  1. Does this PR affect user? Which part will be affected?
  2. What will say in changelog?
  3. Does this PR contains potential break change or other risk?

Changelog description (Optional if not new feature)

  1. English description
  2. Chinese description (optional)

Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

Additional Plan? (Optional if not new feature)

If this PR related with other PR or following info. You can type here.

@netlify
Copy link

netlify bot commented Feb 11, 2019

Deploy preview for ant-design ready!

Built with commit bf135c1

https://deploy-preview-14777--ant-design.netlify.com

@vinhnph-ibl
Copy link
Contributor Author

vinhnph-ibl commented Feb 11, 2019

Before and after
Before and after

@vinhnph-ibl vinhnph-ibl reopened this Feb 11, 2019
@vinhnph-ibl
Copy link
Contributor Author

Please have a review here, @afc163

export type ColumnFilterItem = { text: string; value: string; children?: ColumnFilterItem[] };
export type CompareFn<T> = (a: T, b: T, sortOrder?: SortOrder) => number;
export type ColumnFilterItem = {
text: string | React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

React.ReactNode contains string.

export type ColumnFilterItem = { text: string; value: string; children?: ColumnFilterItem[] };
export type CompareFn<T> = (a: T, b: T, sortOrder?: SortOrder) => number;
export type ColumnFilterItem = {
text: string | React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

Just text: React.ReactNode, React.ReactNode has already included string.

sortOrder?: SortOrder;
},
) => React.ReactNode);
| ((options: { filters: TableStateFilters; sortOrder?: SortOrder }) => React.ReactNode);
Copy link
Member

Choose a reason for hiding this comment

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

Why these style changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's eslint automatically fixed

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #14777 into feature will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           feature   #14777   +/-   ##
========================================
  Coverage    92.79%   92.79%           
========================================
  Files          236      236           
  Lines         6231     6231           
  Branches      1868     1866    -2     
========================================
  Hits          5782     5782           
- Misses         446      448    +2     
+ Partials         3        1    -2
Impacted Files Coverage Δ
components/transfer/index.tsx 85.43% <0%> (ø) ⬆️
components/time-picker/index.tsx 81.94% <0%> (ø) ⬆️

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 2eddc2a...bf135c1. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #14777 into feature will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           feature   #14777   +/-   ##
========================================
  Coverage    92.79%   92.79%           
========================================
  Files          236      236           
  Lines         6231     6231           
  Branches      1868     1866    -2     
========================================
  Hits          5782     5782           
- Misses         446      448    +2     
+ Partials         3        1    -2
Impacted Files Coverage Δ
components/transfer/index.tsx 85.43% <0%> (ø) ⬆️
components/time-picker/index.tsx 81.94% <0%> (ø) ⬆️

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 2eddc2a...bf135c1. Read the comment docs.

@yesmeck yesmeck merged commit d30b2f5 into ant-design:feature Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants