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

fix: form onValues second params values #582

Merged
merged 6 commits into from
Apr 24, 2023

Conversation

elevensky
Copy link
Contributor

@vercel
Copy link

vercel bot commented Apr 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
field-form ❌ Failed (Inspect) Apr 23, 2023 11:46am

@elevensky
Copy link
Contributor Author

elevensky commented Apr 10, 2023

@zombieJ 点Vercel 发布 failed details 404是啥情况?
image

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Merging #582 (1cb2178) into master (c2a6427) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 1cb2178 differs from pull request most recent head 45e1676. Consider uploading reports for the commit 45e1676 to get more accurate results

@@           Coverage Diff           @@
##           master     #582   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          16       16           
  Lines        1143     1146    +3     
  Branches      257      258    +1     
=======================================
+ Hits         1141     1144    +3     
  Misses          2        2           
Impacted Files Coverage Δ
src/Field.tsx 100.00% <100.00%> (ø)
src/List.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zombieJ
Copy link
Member

zombieJ commented Apr 11, 2023

要加一下对应的测试用例哈~

@elevensky
Copy link
Contributor Author

要加一下对应的测试用例哈~

@zombieJ 看看ok不

@zombieJ
Copy link
Member

zombieJ commented Apr 12, 2023

ant-design/ant-design#41053 (comment) 这个看过了么?

@elevensky
Copy link
Contributor Author

elevensky commented Apr 12, 2023

ant-design/ant-design#41053 (comment) 这个看过了么?

@zombieJ 看了,不过总有人会忘了加 {...field},所以我加了默认的,然后测试用例模拟了下他那个例子

src/Field.tsx Outdated Show resolved Hide resolved
src/Field.tsx Outdated Show resolved Hide resolved
src/ListContext.ts Outdated Show resolved Hide resolved
src/Field.tsx Outdated
@@ -625,7 +626,8 @@ class Field extends React.Component<InternalFieldProps, FieldState> implements F

function WrapperField<Values = any>({ name, ...restProps }: FieldProps<Values>) {
const fieldContext = React.useContext(FieldContext);

const listContext = React.useContext(ListContext);
const mergedIsListField = restProps.isListField || (!restProps.isList && !!listContext);
Copy link
Member

Choose a reason for hiding this comment

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

这边这个 isList 的用途是什么哈?

Copy link
Contributor Author

@elevensky elevensky Apr 17, 2023

Choose a reason for hiding this comment

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

image

@zombieJ 原来的代码是用来指定list本身的标识吧,我理解的它本身不属于inListField

Copy link
Member

Choose a reason for hiding this comment

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

这个是告诉 Field 当做 List 处理,不过这里我感觉是不需要的。因为它其实自动注入 isListField

Copy link
Contributor Author

@elevensky elevensky Apr 18, 2023

Choose a reason for hiding this comment

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

@zombieJ 得加的吧,要不List的name收集值不就跳过去了么,
企业微信截图_f82792f7-7897-4f4b-a061-cda053cae6fc
image

Copy link
Member

@zombieJ zombieJ Apr 19, 2023

Choose a reason for hiding this comment

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

如果加了,对于直接 List 嵌套 List 的情况,这个 isListField 就没了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯,明白了,我想想

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zombieJ List里面的Field也给个默认值,根据是否嵌套在List里面,看下还有啥问题没考虑到不。

@elevensky
Copy link
Contributor Author

@zombieJ 改了,再看看

@zombieJ zombieJ merged commit c6dab30 into react-component:master Apr 24, 2023
@elevensky elevensky deleted the fix-onChange branch April 25, 2023 01:25
@closertb
Copy link

这个pr改动,在某个场景下,会引起bug @elevensky @zombieJ ,具体描述看这里:#597

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.

onValuesChange 收集到的数据不准确
3 participants