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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: date range picker type interface #19421

Merged

Conversation

JennieJi
Copy link
Contributor

@JennieJi JennieJi commented Oct 25, 2019

馃 This is a ...

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

馃敆 Related issue link

No related issue found.

馃挕 Background and solution

While using RangePicker in my own project, I find some types of props are missing, and some cannot match with document. So I try to complete the type check on this component in this PR first, then maybe later try to update document accordingly.

馃摑 Changelog

No changes.

Language Changelog
馃嚭馃嚫 English
馃嚚馃嚦 Chinese

鈽戯笍 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

Copy link

@tests-checker tests-checker bot left a comment

Could you please add tests to make sure this change works as expected?

@netlify
Copy link

netlify bot commented Oct 25, 2019

Deploy preview for ant-design ready!

Built with commit 53c2af4

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

@@ -68,10 +72,13 @@ export type RangePickerValue =
export type RangePickerPresetRange = RangePickerValue | (() => RangePickerValue);

export interface RangePickerProps extends PickerProps {
localeCode?: string;
Copy link
Member

@afc163 afc163 Oct 25, 2019

Choose a reason for hiding this comment

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

Is localeCode a RangePicker Prop?

Copy link
Contributor Author

@JennieJi JennieJi Oct 25, 2019

Choose a reason for hiding this comment

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

From code level seems yes, in renderRangePicker it gets localCode from this.props

Copy link
Member

@afc163 afc163 Oct 25, 2019

Choose a reason for hiding this comment

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

But as users of antd, we cannot use this.

Copy link
Contributor Author

@JennieJi JennieJi Oct 25, 2019

Choose a reason for hiding this comment

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

ohhh let me check later

Copy link
Contributor Author

@JennieJi JennieJi Oct 26, 2019

Choose a reason for hiding this comment

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

Hi @afc163 , I have to add @ts-ignore here to avoid typescript complain. But you are right, this localeCode does not work at all, then do you know why there's fixLocale(value, localeCode) in render?

Copy link
Contributor

@dengfuping dengfuping Nov 7, 2019

Choose a reason for hiding this comment

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

  • localCode might be compatible with previous version and it's not recommended to use it.

Copy link
Contributor

@yoyo837 yoyo837 Nov 7, 2019

Choose a reason for hiding this comment

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

compatible锛焛ncompatible?

@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #19421 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19421      +/-   ##
==========================================
- Coverage    97.8%    97.8%   -0.01%     
==========================================
  Files         284      282       -2     
  Lines        7698     7641      -57     
  Branches     2117     2161      +44     
==========================================
- Hits         7529     7473      -56     
+ Misses        169      168       -1
Impacted Files Coverage 螖
components/date-picker/utils.ts 100% <酶> (酶) 猬嗭笍
components/date-picker/RangePicker.tsx 95.88% <100%> (+0.2%) 猬嗭笍
components/input/TextArea.tsx 96.49% <0%> (-3.51%) 猬囷笍
components/modal/confirm.tsx 94.54% <0%> (-0.2%) 猬囷笍
components/alert/index.tsx 93.44% <0%> (-0.11%) 猬囷笍
components/avatar/index.tsx 98.38% <0%> (-0.06%) 猬囷笍
components/transfer/index.tsx 98.7% <0%> (-0.05%) 猬囷笍
components/breadcrumb/Breadcrumb.tsx 96.96% <0%> (-0.05%) 猬囷笍
components/form/FormItem.tsx 98.86% <0%> (-0.01%) 猬囷笍
components/upload/Upload.tsx 99.21% <0%> (-0.01%) 猬囷笍
... and 6 more

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 e88f021...53c2af4. Read the comment docs.

@JennieJi JennieJi force-pushed the jennie/fix-date-range-picker-prop-types branch from ac24f6b to 8409ff9 Compare Oct 26, 2019
@orzyyyy
Copy link
Contributor

orzyyyy commented Oct 26, 2019

Please update your branch to fix lint problem. :)

@JennieJi JennieJi force-pushed the jennie/fix-date-range-picker-prop-types branch from 8409ff9 to 53c2af4 Compare Nov 4, 2019
@dengfuping
Copy link
Contributor

dengfuping commented Nov 7, 2019

localCode might be compatible with previous version and it's not recommended to use it.

  • Sorry to close it and thanks for your contribution.

@dengfuping dengfuping closed this Nov 7, 2019
@afc163 afc163 reopened this Nov 7, 2019
@afc163 afc163 merged commit 948a69c into ant-design:master Nov 7, 2019
23 checks passed
@yoyo837
Copy link
Contributor

yoyo837 commented Nov 7, 2019

馃槺 鎰忚鐩稿乏銆侭ased on @dengfuping's point of view, what is the reason for the merge? 鐪嬫嚨銆

@afc163
Copy link
Member

afc163 commented Nov 7, 2019

localCode is not included in changes.

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

5 participants