Refactor list tsx #1280

himStone · 2017-05-08T11:58:07Z

First of all, thanks for your contribution! :-)

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

Make sure 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.

This change is

#1212

cncolder · 2017-05-08T14:46:11Z

赞一个，个人认为 Typescript 项目除了编译时检查以外，对 IDE 的支持意义更大。

codecov · 2017-05-08T16:33:37Z

Codecov Report

Merging #1280 into master will increase coverage by 0.02%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #1280      +/-   ##
==========================================
+ Coverage   61.75%   61.77%   +0.02%     
==========================================
  Files         219      219              
  Lines        4165     4165              
  Branches     1225     1224       -1     
==========================================
+ Hits         2572     2573       +1     
+ Misses       1592     1591       -1     
  Partials        1        1

Flag	Coverage Δ
#rn	`63.17% <95.45%> (+0.04%)`	⬆️
#web	`60.35% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
components/list/index.web.tsx	`100% <ø> (ø)`	⬆️
components/list-view/handleProps.tsx	`0% <ø> (ø)`	⬆️
components/list/index.tsx	`82.6% <100%> (-1.77%)`	⬇️
components/list/ListItem.web.tsx	`70.17% <100%> (ø)`	⬆️
components/list/ListItem.tsx	`98.36% <95.23%> (+0.28%)`	⬆️

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 37fb7de...e29925a. Read the comment docs.

himStone · 2017-05-08T16:41:54Z

@cncolder SSR我帮不上什么忙，挑简单的整 😊

paranoidjk · 2017-05-09T04:48:29Z

components/list/ListItem.tsx

  render() {
    const {
-      styles, children, multipleLine, thumb, extra, arrow = '', style, onClick,
+      styles = listItemStyles, children = {}, multipleLine, thumb, extra, arrow = '', style, onClick,


设 defaultProps, 不建议解构的时候设默认值

[ts] Object is possibly 'undefined'

不在解构的时候设置的话，typescript会报错，有什么好办法么

https://github.com/ant-design/ant-design-mobile/blob/master/components/grid/index.web.tsx#L78 目前可以这样绕过.

后续可以跟踪typescript是否会处理这个问题

其实想了下 [ts] Object is possibly 'undefined' 应该是对的，用户可能设置 props.styles = undefined, 除非 typescript 结合 props interface 的类型以及 defaultProps 来推断

嗯那defaultProps和解构的时候都应该设置默认值

一般不用，大部分逻辑下我们应该不需要拦截props.foo = undefined这种场景吧

paranoidjk · 2017-05-09T04:51:14Z

components/list-view/handleProps.tsx

@@ -1,7 +1,7 @@
 /* tslint:disable:no-unused-variable */
 import React from 'react';
 /* tslint:enable:no-unused-variable */
-import List from '../list';


@cncolder 这里在构建的时候是没问题的，有 webpack 配置：

resolve: { modulesDirectories: ['node_modules', path.join(__dirname, '../node_modules')], extensions: ['', '.web.js', '.js', '.json'], },

typescript或者vscode有办法识别码？

目前因为有计划2.0改后缀名 #1235

所以能不加.web的希望尽量不加

@paranoidjk vscode 不会去识别 webpack 配置的，vscode 只认同名的 .d.ts 文件和 node_modules/@types/*

@paranoidjk 这个后缀目前怎么说，typescript的module resolver好像没有地方可以配置

那就加吧

好嘞现在解构的地方做了修改，估计之前也是因为ts的关系做了很多hack

paranoidjk · 2017-05-10T03:40:35Z

components/list/ListItem.tsx

  render() {
    const {
-      styles, children, multipleLine, thumb, extra, arrow = '', style, onClick,
-      onPressIn = noop, onPressOut = noop, wrap, disabled, align, ...restProps,
+      styles = listItemStyles, children, multipleLine, thumb, extra, arrow, style, onClick,


有 defaultProps 就不要设解构的默认值了，太冗余。
typescript的warning另想办法处理，比如 as any，当然可以看看有没有更好方案

paranoidjk · 2017-05-10T03:44:14Z

components/list/index.tsx

@@ -42,14 +23,14 @@ export default class List extends React.Component<ListProps, any> {
      headerDom = <View>{content}</View>;
    }
    if (renderFooter) {
-      let content = typeof renderHeader === 'function' ? renderFooter() : renderFooter;
+      let content = typeof renderFooter === 'function' ? renderFooter() : renderFooter;


这是 bug fix 吧... cc @silentcloud

顺手改了...忘记分开commit了....

没拆开会给code review增加难度...

涉及到不同逻辑的改动还是拆开

paranoidjk · 2017-05-10T03:47:16Z

components/checkbox/CheckboxItem.tsx

@@ -37,7 +37,6 @@ export default class CheckboxItem extends React.Component<CheckboxItemProps, any
      <ListItem
        style={style}
        onClick={disabled ? undefined : this.handleClick}
-        line={line}


这里还是个多余的props..

cc @silentcloud

paranoidjk · 2017-05-10T03:53:06Z

@bccsafe bug fix 和逻辑的改动拆出来单独 PR，这个只做 props 的重构

himStone · 2017-05-10T03:53:44Z

@paranoidjk 好的

himStone · 2017-05-10T16:13:57Z

@paranoidjk

解构默认值的问题，可以暂时用这个操作符规避
当然是要在确保不会undefined的情况下

styles.underlayColor -> styles!.underlayColor

https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#non-null-assertion-operator

A new ! post-fix expression operator may be used to assert that its operand is non-null and non-undefined in contexts where the type checker is unable to conclude that fact. Specifically, the operation x! produces a value of the type of x with null and undefined excluded. Similar to type assertions of the forms x and x as T, the ! non-null assertion operator is simply removed in the emitted JavaScript code.

himStone · 2017-05-12T05:49:47Z

ping

paranoidjk · 2017-05-12T06:17:01Z

components/list/ListItem.tsx

@@ -59,17 +76,17 @@ export default class Item extends React.Component<ListItemProps, any> {
        if (React.isValidElement(el)) {
          tempContentDom.push(el);
        } else {
-          tempContentDom.push(<Text style={[styles.Content]} {...numberOfLines} key={`${index}-children`}>{el}</Text>);
+          tempContentDom.push(<Text style={[styles!.Content]} {...numberOfLines} key={`${index}-children`}>{el}</Text>);


这个感觉代码侵入性太强了，一堆！散落在代码里以后不好维护

个人感觉还不如在最前面用 as any

as any 也得 (styles as any).Content

还不如styles!，而且现在要去any ant-design/ant-design#5627

不是在每次取值的时候加 as，取值的时候做一次 https://github.com/ant-design/ant-design-mobile/blob/master/components/grid/index.web.tsx#L30

const anyStyles = styles as any;

如果这样，之后styles的类型改了，很可能没法第一时间发现问题，但是!不会，他只是用于确保不会null | undefined

在这次重构list的propType时，就发现了三个bug，这些bug如果声明严格，在编辑器上就能直接发现

himStone · 2017-05-13T08:08:46Z

@paranoidjk

突然想到这样不就好了....

https://github.com/ant-design/ant-design-mobile/pull/1280/files#diff-58efeb14d04312904e826f612da850f2R39

himStone · 2017-05-15T11:10:08Z

ping ...

paranoidjk · 2017-05-15T11:11:07Z

我看看，一直在忙 😓

paranoidjk · 2017-05-15T11:11:31Z

@silentcloud 也 review 下。

silentcloud · 2017-05-15T11:13:58Z

cc @pingan1927

ArtemSerga · 2017-06-14T12:16:41Z

components/baseType.tsx

@@ -0,0 +1,14 @@
+import React from 'react';
+import { ViewStyle } from 'react-native';


This import raises TS compile error in TS WEB projects

ERROR in /Users/aserga/www/antd-mobile-samples/web-typescript/node_modules/antd-mobile/lib/baseType.d.ts (3,27): error TS2307: Cannot find module 'react-native'.

This should be fixed in #1376 @bccsafe

himStone force-pushed the refactor-list-tsx branch from 8d2058c to 17a03ce Compare May 8, 2017 16:33

himStone changed the title ~~[WIP]Refactor list tsx~~ Refactor list tsx May 8, 2017

paranoidjk requested review from silentcloud and cncolder May 9, 2017 04:41

paranoidjk reviewed May 9, 2017

View reviewed changes

himStone force-pushed the refactor-list-tsx branch from 17a03ce to 7369051 Compare May 9, 2017 16:38

paranoidjk reviewed May 10, 2017

View reviewed changes

himStone changed the title ~~Refactor list tsx~~ [WIP] Refactor list tsx May 10, 2017

himStone force-pushed the refactor-list-tsx branch from 7369051 to ff57d4a Compare May 10, 2017 16:08

himStone changed the title ~~[WIP] Refactor list tsx~~ Refactor list tsx May 10, 2017

paranoidjk reviewed May 12, 2017

View reviewed changes

refactor list tsx

e29925a

himStone force-pushed the refactor-list-tsx branch from 2a0ed10 to e29925a Compare May 13, 2017 08:00

paranoidjk merged commit 9d9890e into ant-design:master May 15, 2017

himStone deleted the refactor-list-tsx branch May 15, 2017 11:47

silentcloud mentioned this pull request May 22, 2017

native type should be same with web type #1339

Closed

ArtemSerga reviewed Jun 14, 2017

View reviewed changes

lixiaoyang1992 pushed a commit to lixiaoyang1992/ant-design-mobile that referenced this pull request Apr 26, 2018

refactor list tsx (ant-design#1280)

1eaf87a

		@@ -0,0 +1,14 @@
		import React from 'react';
		import { ViewStyle } from 'react-native';

Refactor list tsx #1280

Refactor list tsx #1280

Conversation

himStone commented May 8, 2017 • edited Loading

cncolder commented May 8, 2017

codecov bot commented May 8, 2017 • edited Loading

Codecov Report

himStone commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranoidjk May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paranoidjk commented May 10, 2017

himStone commented May 10, 2017

himStone commented May 10, 2017

himStone commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himStone commented May 13, 2017

himStone commented May 15, 2017

paranoidjk commented May 15, 2017

paranoidjk commented May 15, 2017

silentcloud commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himStone commented May 8, 2017 •

edited

Loading

codecov bot commented May 8, 2017 •

edited

Loading

paranoidjk May 9, 2017 •

edited

Loading