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

refactor type definitions #2323

Merged
merged 62 commits into from Mar 16, 2018
Merged

refactor type definitions #2323

merged 62 commits into from Mar 16, 2018

Conversation

BANG88
Copy link
Member

@BANG88 BANG88 commented Feb 5, 2018

First of all, thank you for your contribution! :-)

Please makes sure that these checkboxes are checked before submitting your PR, thank you!

  • Make sure that you follow antd's code convention.
  • Run npm run lint and fix those errors before submitting in order to keep consistent code style.
  • Rebase before creating a PR to keep commit history clear.
  • Add some descriptions and refer relative issues for you PR.

Extra checklist:

if isBugFix :

  • Make sure that you add at least one unit test for the bug which you had fixed.

elif isNewFeature :

  • Update API docs for the component.
  • Update/Add demo to demonstrate new feature.
  • Update TypeScript definition for the component.
  • Add unit tests for the feature.

Don't merge this is a WIP PR. since it have too many changes. PR here for discussion #2315


This change is Reviewable

@BANG88 BANG88 mentioned this pull request Feb 5, 2018
@codecov
Copy link

codecov bot commented Feb 5, 2018

Codecov Report

Merging #2323 into master will decrease coverage by 3.85%.
The diff coverage is 55.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
- Coverage   58.74%   54.89%   -3.86%     
==========================================
  Files         259      145     -114     
  Lines        4380     2525    -1855     
  Branches     1157      755     -402     
==========================================
- Hits         2573     1386    -1187     
+ Misses       1806     1134     -672     
- Partials        1        5       +4
Flag Coverage Δ
#rn ?
#web 54.89% <55.6%> (-2.05%) ⬇️
Impacted Files Coverage Δ
components/input-item/PropsType.tsx 0% <ø> (ø) ⬆️
components/modal/index.tsx 100% <ø> (ø) ⬆️
components/flex/PropsType.tsx 0% <ø> (ø) ⬆️
components/menu/SubMenu.tsx 100% <ø> (ø) ⬆️
components/calendar/PropsType.tsx 0% <ø> (ø) ⬆️
components/menu/PropsType.tsx 0% <ø> (ø) ⬆️
components/tab-bar/PropsType.tsx 0% <ø> (ø) ⬆️
components/drawer/index.tsx 100% <ø> (ø) ⬆️
components/list-view/PropsType.tsx 0% <ø> (ø) ⬆️
components/result/PropsType.tsx 0% <ø> (ø) ⬆️
... and 182 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 29638b9...5db26f3. Read the comment docs.

@BANG88 BANG88 changed the title [WIP] refactor type definitions refactor type definitions Feb 7, 2018
@BANG88
Copy link
Member Author

BANG88 commented Mar 10, 2018

Ping @paranoidjk

@paranoidjk
Copy link
Contributor

@BANG88 👍 我看看。

加个微信吧, 我的 id: paranoidboy 😄

@@ -28,11 +37,19 @@ class ActionSheetAndroid extends React.Component<IActionSheetNativeProps, any> {
visible: false,
});
}

close = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we have a bug here?

cc @silentcloud

@@ -7,6 +7,7 @@
"noUnusedLocals": true,
"allowSyntheticDefaultImports":true,
"target": "es6",
"noImplicitAny": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

So we can close #2147

Copy link
Contributor

Choose a reason for hiding this comment

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

But i searched the codebase, there still have many any left

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. some of them needs enhancement.

@@ -124,68 +123,77 @@
"webpack-visualizer-plugin": "^0.1.11"
},
"scripts": {
"lint": "npm run tslint && npm run srclint && npm run demolint && npm run stylelint && npm run applint",
"lint":
"npm run tslint && npm run srclint && npm run demolint && npm run stylelint && npm run applint",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why JSON value need to start at newline? looks like is unusual usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Miss... formatted automatically by VSCode..

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe because its too long for read. 😄

});

return (
<span className={badgeCls}>
{children}
{(text || dot) && <sup className={scrollNumberCls} {...restProps}>{text}</sup>}
{(text || dot) && (
// tslint:disable-next-line:jsx-no-multiline-js
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can disable jsx-no-multiline-js rules, since actually we have lots of usage like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should disable it via ant tools.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer prettier for code formatting.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can PR to https://github.com/ant-design/antd-tools/blob/master/lib/tslint.json to disable this rule. cc @yesmeck Do you argree to disable this in antd?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, prettier +1

@@ -120,7 +120,9 @@ export default class ImagePicker extends React.Component<
this.parseFile(files[i], i);
}
}
fileSelectorEl.value = '';
if (fileSelectorEl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have merge #2302.

So that we may not need Defensive programming here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It maybe null reference so need have a check here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since merge #2302 fileSelectorEl should always reference to input element instance once component mount.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep but the type of ref are Element | null. TypeScript checker can not pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use fileSelectorEl!.value = '';?Since if null check is a runtime issue. If we only need to let ts compiler skip this, we may use exclamation mark.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like !. may forget check for some reason sometimes

@@ -301,7 +300,7 @@ exports[`renders ./components/carousel/demo/basic.native.tsx correctly 1`] = `
"justifyContent": "center",
},
Object {
"backgroundColor": "red",
"backgroundColor": "#ccc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there is several snapshot change in this PR? I think type definition change should not have impact on ui.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. but i forgot why this has been changed. 😢

@paranoidjk
Copy link
Contributor

@BANG88 #2339


export interface IconPropType {
type: string;
export interface IconProps extends IconPropsType {
Copy link
Contributor

Choose a reason for hiding this comment

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

继承 SVGAttributes#2339

@paranoidjk
Copy link
Contributor

@BANG88 解决冲突。另外尽量只改类型不改业务逻辑,业务逻辑有问题需要改的的单独 PR。

* master:
  test: update snapshot
  ci: upgrade to nodejs 8
  fix(Carousel): ant-design#2369
  chore: delete upgrade tip
  fix: Fix all failed test (ant-design#2337)
  fix: modal.alert content align=center (ant-design#2347)
  bugfix: fixed bug in image-picker, Uncaught TypeError: Cannot set property 'value' of null (ant-design#2302)

# Conflicts:
#	components/_util/upgradeTip.tsx
#	components/image-picker/index.tsx
@BANG88
Copy link
Member Author

BANG88 commented Mar 15, 2018

👌

@paranoidjk paranoidjk merged commit 77347dd into ant-design:master Mar 16, 2018
@paranoidjk paranoidjk mentioned this pull request Mar 19, 2018
9 tasks
lixiaoyang1992 pushed a commit to lixiaoyang1992/ant-design-mobile that referenced this pull request Apr 26, 2018
* refactor(util): noImplicitAny

* refactor(accordion): type definitions improvements

* refactor(action-sheet): add missing type definitions

* chore: configuration

* refactor(activity-indicator): type definitions

* chore: configuration

* refactor(badge): type definitions

* refactor(button): type definitions

* refactor(calendar): type definitions

* refactor(card): type definitions

* refactor(carousel): add type definitions and refactor native part

* refactor(checkbox): type definitions

* refactor(list): type definitions

* refactor(date-picker): type definitions

* refactor(date-picker-view): type definitions

* refactor(drawer): type definitions

* refactor(flex): type definitions

* refactor(grid): type definitions

* refactor(icon): type definitions

* refactor(image-picker): type definitions

* refactor(input-item): type definitions and code refactor

* refactor(locale-provider): type definitions

* refactor(menu): type definitions

* refactor(modal): type definitions

* refactor(nav-bar): type definitions

* refactor(notice-bar): type definitions

* refactor(pagination): type definitions

* refactor(picker): type definitions

* refactor(picker-view): type definitions

* refactor(popover): type definitions

* refactor(progress): type definitions

* refactor(radio): type definitions

* refactor(range): type definitions

* refactor(result): type definitions

* refactor(search-bar): type definitions

* refactor(segmented-control): type definitions

* refactor(slider): type definitions

* refactor(stepper): type definitions

* refactor(steps): type definitions

* refactor(swipe-action): type definitions

* refactor(switch): type definitions

* refactor(tab-bar): type definitions

* refactor(tabs): type definitions

* refactor(tag): type definitions

* refactor(view|text): type definitions

* refactor(textarea-item): type definitions

* refactor(toast): type definitions

* refactor(white-space): type definitions

* refactor(wing-blank): type definitions

* refactor(list-view): type definitions

* refactor(demo): any

* chore(style): use ant-tools tslint configurations

* test: update snapshots

* chore: remove unnecessary configurations and deps

* chore: tslint

* chore: add setup file

* fix: ant-design#2302

* test: update snap

* chore: update deps and  fix tests

* chore: try to let jest faster

* refactor(Icon): extends from SVG Props
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants