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

[WIP] add tslint type check test #1560

Closed
wants to merge 10 commits into from
Closed

Conversation

cncolder
Copy link
Contributor

@cncolder cncolder commented Jul 12, 2017

try refactoring list type definition

This pr use for discussion. Working in progress. DONOT merge.

ref #1557
ref ant-design/antd-mobile-samples#21

After this pr. React Native user need update tsconfig.json

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "antd-mobile": [
        "./node_modules/antd-mobile/lib/native"
      ]
    }
  }
}

Once you finish configure path mapping. You will get nearly perfect vscode IntelliSense for List component.

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.

This change is Reviewable

@cncolder cncolder added this to the 2.0.0 milestone Jul 12, 2017
@mention-bot
Copy link

@cncolder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pingan1927, @yiminghe and @paranoidjk to be potential reviewers.

pingan1927 and others added 2 commits July 13, 2017 14:54
* feat: pagination current start from 1. close ant-design#1009

* refactor style for some components, ref ant-design#1380

* fix flex ant-design#1532

* update snap

* modify CHANGELOG
@codecov
Copy link

codecov bot commented Jul 13, 2017

Codecov Report

Merging #1560 into feature-2.0 will increase coverage by 0.04%.
The diff coverage is 76.19%.

Impacted file tree graph

@@               Coverage Diff               @@
##           feature-2.0    #1560      +/-   ##
===============================================
+ Coverage         66.1%   66.15%   +0.04%     
===============================================
  Files              226      226              
  Lines             2269     2272       +3     
  Branches           696      699       +3     
===============================================
+ Hits              1500     1503       +3     
  Misses             768      768              
  Partials             1        1
Flag Coverage Δ
#rn 82.07% <ø> (ø) ⬆️
#web 65.37% <76.19%> (+0.04%) ⬆️
Impacted Files Coverage Δ
components/textarea-item/index.tsx 100% <ø> (ø) ⬆️
components/switch/index.tsx 100% <ø> (ø) ⬆️
components/search-bar/index.tsx 100% <ø> (ø) ⬆️
components/progress/index.tsx 100% <ø> (ø) ⬆️
components/pagination/index.tsx 100% <ø> (ø) ⬆️
components/list/index.web.tsx 100% <ø> (ø) ⬆️
components/search-bar/index.web.tsx 78.57% <ø> (ø) ⬆️
components/list/index.tsx 100% <ø> (ø) ⬆️
components/textarea-item/index.web.tsx 45.26% <ø> (ø) ⬆️
components/list/ListItem.tsx 100% <ø> (ø) ⬆️
... and 11 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 851e60c...4c6b009. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 13, 2017

Codecov Report

Merging #1560 into feature-2.0 will decrease coverage by 0.78%.
The diff coverage is 100%.

Impacted file tree graph

@@               Coverage Diff               @@
##           feature-2.0    #1560      +/-   ##
===============================================
- Coverage         66.1%   65.32%   -0.79%     
===============================================
  Files              226      120     -106     
  Lines             2269     2163     -106     
  Branches           696      604      -92     
===============================================
- Hits              1500     1413      -87     
+ Misses             768      749      -19     
  Partials             1        1
Flag Coverage Δ
#rn ?
#web 65.32% <100%> (ø) ⬆️
Impacted Files Coverage Δ
components/list/index.web.tsx 100% <ø> (ø) ⬆️
components/list/ListItem.web.tsx 70.17% <100%> (ø) ⬆️
components/flex/FlexItem.tsx
components/image-picker/index.tsx
components/notice-bar/Marquee.tsx
components/style/themes/default.tsx
components/date-picker/utils.tsx
components/switch/index.tsx
components/result/index.tsx
components/icon/index.tsx
... and 96 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 851e60c...004ea78. Read the comment docs.

try refactoring list type definition
@@ -1,74 +1,77 @@
import React, { ReactNode } from 'react';
import React from 'react';
import ReactNative from 'react-native';
Copy link
Contributor

Choose a reason for hiding this comment

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

这里还是会导致一个目前已知的问题

  • 使用 web 组件的用户需要安装 react-native 依赖

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个很好解决,WebProps 和 NativeProps 移到各自的组件里。

类型定义在需要的地方没什么不可以的,因为我们本来就是个 typescript 项目。 PropsType.tsx 存在的意义是重用代码,而且 PropsType 这个文件名也待考量,我们使用 typescript 的目的不能只局限于 React props type 验证这个低起点上。

@@ -0,0 +1,34 @@
import List from 'antd-mobile/lib/list';
Copy link
Contributor

Choose a reason for hiding this comment

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

按你这个改法,这一行 https://github.com/ant-design/ant-design-mobile/blob/feature-2.0/components/index.tsx#L1 的注释可以去掉了,umd dist 打包的入口和 types 文件生成的入口已经拆分开了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个不是改动,是因为我只试着重写了 List 的类型定义,全部导入的话测试通不过,这个文件是为了 tslint --type-check 用的。

用户还是要 import {} from 'antd-mobile'

@@ -11,7 +11,8 @@
"exclude": [
"node_modules",
"lib",
"es"
"es",
Copy link
Contributor

Choose a reason for hiding this comment

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

还要改 Pkg.typings 吧?默认指向 web 的类型文件,rn的用户就按照 ant-design/antd-mobile-samples#21
自定义 tsconfig 指向 native 的类型文件

@paranoidjk
Copy link
Contributor

@cncolder
分支已经切过来了。

https://github.com/ant-design/ant-design-mobile/tree/master -> 2.x
https://github.com/ant-design/ant-design-mobile/tree/1.x -> 1.x

直接往 master 合吧。

@paranoidjk paranoidjk closed this Jul 31, 2017
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