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

allow multi element in children #1442

Merged
merged 1 commit into from Jun 14, 2017
Merged

allow multi element in children #1442

merged 1 commit into from Jun 14, 2017

Conversation

cncolder
Copy link
Contributor

@cncolder cncolder commented Jun 14, 2017

If set children type. We need allow element array.

<Flex>
  <Flex.Item />
  <Flex.Item />
</Flex>
  Type '{ children: Element[]; }' is not assignable to type 'Readonly<FlexWebProps>'.
    Types of property 'children' are incompatible.
      Type 'Element[]' is not assignable to type 'Element | undefined'.
        Type 'Element[]' is not assignable to type 'Element'.
          Property 'type' is missing in type 'Element[]'.'
at: '241,13'
source: 'ts'

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 requested a review from himStone June 14, 2017 09:21
@codecov
Copy link

codecov bot commented Jun 14, 2017

Codecov Report

Merging #1442 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1442   +/-   ##
=======================================
  Coverage   63.44%   63.44%           
=======================================
  Files         222      222           
  Lines        4336     4336           
  Branches     1272     1272           
=======================================
  Hits         2751     2751           
  Misses       1584     1584           
  Partials        1        1
Flag Coverage Δ
#rn 64.29% <ø> (ø) ⬆️
#web 62.62% <ø> (ø) ⬆️

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 3f5c961...4d1f83a. Read the comment docs.

@@ -5,7 +5,7 @@ export interface FlexProps {
wrap?: 'nowrap'|'wrap'|'wrap-reverse';
justify?: 'start'|'end'|'center'|'between'|'around';
align?: 'top'|'start'|'middle'|'center'|'bottom'|'end'|'baseline'|'stretch';
children?: JSX.Element;
children?: JSX.Element | JSX.Element[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am just wonder where is JSX come from ? does it is typescript base type?

Copy link
Contributor Author

@cncolder cncolder Jun 14, 2017

Choose a reason for hiding this comment

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

// @types/react/index.d.ts
declare global {
    namespace JSX {
        interface Element extends React.ReactElement<any> { }

Copy link
Contributor Author

@cncolder cncolder Jun 14, 2017

Choose a reason for hiding this comment

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

我对 typescript 理解也不深,我认为 JSX.Element 应该能包括所有组件了。但也不能过度使用,例如这里用 React.ReactNode 更合适:

<List>
  <List.Item>abc</List.Item>
</List>
  Type '{ children: string; }' is not assignable to type 'Readonly<ListItemWebProps>'.
    Types of property 'children' are incompatible.
      Type 'string' is not assignable to type 'Element | undefined'.'

Copy link
Contributor

Choose a reason for hiding this comment

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

    interface ReactElement<P> {
        type: string | ComponentClass<P> | SFC<P>;
        props: P;
        key: Key | null;
    }
    type ReactNode = ReactChild | ReactFragment | boolean | null | undefined;
    type ReactChild = ReactElement<any> | ReactText;
    type ReactFragment = {} | Array<ReactChild | any[] | boolean>;
    type ReactChild = ReactElement<any> | ReactText;

Flex 的 children 应该必须是 Flex.Item 数组吧,还是用 JSX.Element | JSX.Element[] 合理

@cncolder
Copy link
Contributor Author

cncolder commented Jun 14, 2017

其实类型定义这块我有很多想改的地方,但是因为涉及到 rn 源代码,所以不敢做大的改动。

这个项目对类型定义的态度是什么?我看到现在基本上只有 PropsType.tsx 这一个文件里是存放存属性类型,源码里没有逻辑代码以外的东西,PropsType.tsx 这个文件里可不可以写除了 props 之外的代码?

比如 Modal.alert,因为实现上的问题,它不能像 Toast.info 那样导出类型,要想实现这个我能想到的是抽象类。想问下现阶段能接受这样的改动么?

// components/modal/PropsType.tsx
export abstract class ModalComponent<P, S> extends React.Component<P, S> {
  static alert: (
    title: string | JSX.Element,
    message: string | JSX.Element,
    actions?: Action[]
  ) => { close: () => void }
}
// components/modal/Modal.web.tsx
import ModalProps, { ModalComponent } from './PropsType';
export default class Modal extends ModalComponent<ModalProps, any> {
}

@paranoidjk
Copy link
Contributor

@cncolder

我 +1 👍 你的方式更符合我们使用 TypeScript 的意义,单独拆分 PropType 应该是最早低成本的快速从 react propType 迁移到 typescript interface 的方式吧

直接 PR 吧~

@paranoidjk paranoidjk merged commit 21e306b into ant-design:master Jun 14, 2017
lixiaoyang1992 pushed a commit to lixiaoyang1992/ant-design-mobile that referenced this pull request Apr 26, 2018
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

2 participants