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

feat: New Form #17327

Merged
merged 55 commits into from
Jul 3, 2019
Merged

feat: New Form #17327

merged 55 commits into from
Jul 3, 2019

Conversation

zombieJ
Copy link
Member

@zombieJ zombieJ commented Jun 26, 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?)

👻 What's the background?

ref #16911

https://github.com/react-component/field-form/

📝 Changelog

Language Changelog
🇺🇸 English New Form!
🇨🇳 Chinese 新的 Form!

☑️ 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

TODO

  • Demo test support tsx test
  • Other Component Demo use Form update
  • Test Coverage

Preview

https://deploy-preview-17327--ant-design.netlify.com/components/form-cn/#header

@pr-triage pr-triage bot added the PR: draft label Jun 26, 2019
@todo
Copy link

todo bot commented Jun 26, 2019

Remove this

// TODO: Remove this
import Form from './Form';
export {


This comment was generated by todo based on a TODO comment in 067abdb in #17327. cc @ant-design.

Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

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

@todo
Copy link

todo bot commented Jun 26, 2019

handle this

[`${prefixCls}-item-with-help`]: domErrorVisible, // TODO: handle this
[`${className}`]: !!className,
// Status
[`${prefixCls}-item-has-feedback`]:
(mergedValidateStatus && hasFeedback) || mergedValidateStatus === 'validating',


This comment was generated by todo based on a TODO comment in 067abdb in #17327. cc @ant-design.

@todo

This comment has been minimized.

@todo

This comment has been minimized.

@zombieJ
Copy link
Member Author

zombieJ commented Jul 2, 2019

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2019

This pull request fixes 1 alert when merging 013281c into f589577 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2019

This pull request fixes 1 alert when merging 0ac86bd into f589577 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 2, 2019

This pull request fixes 1 alert when merging 7705292 into f589577 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@afc163
Copy link
Member

afc163 commented Jul 3, 2019

好像缺 onError

@lgtm-com
Copy link

lgtm-com bot commented Jul 3, 2019

This pull request fixes 1 alert when merging 52d8d48 into f589577 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jul 3, 2019

This pull request fixes 1 alert when merging 80a3ee8 into f589577 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@zombieJ zombieJ merged commit 4cdf37b into 4.0-prepare Jul 3, 2019
@delete-merged-branch delete-merged-branch bot deleted the form branch July 3, 2019 12:14
| hasFeedback | 配合 `validateStatus` 属性使用,展示校验状态图标,建议只配合 Input 组件使用 | boolean | false |
| help | 提示信息,如不设置,则会根据校验规则自动生成 | string\|ReactNode | - |
| htmlFor | 设置子元素 label `htmlFor` 属性 | string | - |
| inline | 为 `true` 时不带样式,作为纯字段控件使用 | boolean | false |
Copy link
Member

Choose a reason for hiding this comment

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

这个容易和样式里的 inline 概念混淆,有没有别的名字。

Copy link
Member Author

Choose a reason for hiding this comment

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

pure?

Copy link
Member

Choose a reason for hiding this comment

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

nostyle 如何

Copy link
Member Author

Choose a reason for hiding this comment

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

noStyle 和现在的 naming standard 不太合,要不然直接 style={false}

Copy link
Member

Choose a reason for hiding this comment

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

style={false} 和其他组件行为不一致,不好。

Copy link
Member Author

Choose a reason for hiding this comment

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

那还是 inline

Copy link
Member

Choose a reason for hiding this comment

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

inline 表意不明,建议还是 noStyle 或者 withoutStyle

> label {
position: relative;
display: inline-block;
margin-top: @form-label-margin-top;

Choose a reason for hiding this comment

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

That line created a misalignment when we use label and input with diff sizes, for ex:

<Input size="large"/>

Copy link
Member Author

Choose a reason for hiding this comment

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

Label position is a fixed value instead of related vertical center of the Item since some component like TextArea (or Transfer or Tree or some else) occupy the more height of it.

For the size diff component, maybe provides some other prop to handle (still in thinking).

ref: https://codesandbox.io/s/hungry-driscoll-zc9cd

}
}

.make-vertical-layout() {
Copy link
Member

Choose a reason for hiding this comment

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

@gavinxgu
Copy link

gavinxgu commented Feb 9, 2020

When will this feature be released ?

@yoyo837
Copy link
Contributor

yoyo837 commented Feb 9, 2020

You can try it by the latest RC version.

Comment on lines +5 to +9
interface FieldData {
name: number;
key: number;
fieldKey: number;
}

Choose a reason for hiding this comment

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

Are these types correct?

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.

8 participants