Skip to content

fix(form): resolve the problem of validation states changing out of sequence#41412

Merged
zombieJ merged 16 commits intomasterfrom
fix-validate-status
Mar 23, 2023
Merged

fix(form): resolve the problem of validation states changing out of sequence#41412
zombieJ merged 16 commits intomasterfrom
fix-validate-status

Conversation

@kiner-tang
Copy link
Copy Markdown
Member

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Workflow
  • Other (about what?)

🔗 Related issue link

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English resolve the problem of validation states changing out of sequence
🇨🇳 Chinese 解决验证状态不按照顺序改变的问题

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • 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

@kiner-tang kiner-tang requested review from MadCcc and zombieJ March 23, 2023 03:59
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 23, 2023

}, [mergedValidateStatus, hasFeedback, desplayValidateStatus]);

// ======================== Render ========================
const itemClassName = classNames(itemPrefixCls, className, rootClassName, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这个代码补丁对一个 React 组件做了一些更改,主要是关于表单中每个输入元素的验证状态及其相关图标的处理。下面是我的一些涵盖此代码补丁的风险和改进建议:

  • 风险:
  1. 行 30 中 debounceErrors.length 应该换成 meta.errors.length, 因为 debounceErrors 并没有在代码中定义或更新,而 meta.errors 包含来自后端或前端的异步或同步错误信息。

  2. 当使用feedbackIcon渲染反馈图标时,请确保图标存在。例如 iconMap 对象可能缺少特定验证状态的图标, 这将导致图标 undefined

  • 改进:
  1. 建议使用新命名的 desplayValidateStatus 来减少在逻辑决策点处的重复逻辑,提高代码可读性和可维护性。

  2. 您可以考虑优化并发请求的性能和资源使用情况。

  3. 如果您的表单较大且有很多输入元素,则建议使用懒加载技术以加快页面加载速度。

expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码是对一个 Ant Design 的表单组件进行的测试。从代码中可以看出,主要是通过渲染组件并触发事件来测试组件的行为,比如输入值、提交表单等。其中的改进建议包括:

  • 在测试中尽可能使用实际的用户场景和数据,以获得更真实的测试结果。
  • 将测试用例分组,以便更好地组织和管理测试用例。
  • 在测试用例中注意添加适当的注释和文档,以便更好地理解测试逻辑和测试目的。

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 23, 2023

size-limit report 📦

Path Size
./dist/antd.min.js 374.67 KB (+45 B 🔺)
./dist/antd-with-locales.min.js 431.54 KB (+39 B 🔺)

expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码是对antd Form组件的测试用例,主要涉及输入框、下拉框、日期选择框等控件的测试,其中包括验证状态变更、布局以及提交表单等方面。通过引入一些工具函数进行验证,例如mountTest、rtlTest、fireEvent、pureRender和render等。建议可以添加更多针对时间和数值范围、上传、穿梭框、自定义元素和徽章等控件的测试。另外,可以优化代码格式,将import语句重新排序以提高可读性,并移除不必要的import语句。

expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这是一个 React 组件库中的 Form 组件的代码补丁。以下是我的简要代码审查:

  • 从组件导入的模块顺序有些混乱,建议按字母顺序排序。
  • mountTestrtlTest模块的导入看起来不属于此组件,建议删除。
  • 该组件导入了一些其他 Ant Design 组件如 Button, Drawer, Modal, Select, Upload等,如果这些组件没有被使用,建议删除以减小代码量。
  • 在测试方面,则存在一个测试用例来测试验证状态是否会按顺序更改,对于验证代码关键路径的覆盖率很有意义。

对于提高代码质量和性能的改进:

  • 建议将组件内的无状态(缺少状态)的函数 component(FC)替换为纯函数组件或更好地使用 hooks(具体方法取决于它们的实际情况)。这可以提高组件的性能和可维护性。
  • 对于资源消耗,尝试避免重复呈现组件或不必要的重新渲染。可以使用 React.memo 来优化组件的渲染,或 通过应用 shouldComponentUpdatePureComponent 来优化类组件的渲染。

expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码import了一些antd和react组件以及一些测试用的工具函数,包含一个表单组件Form的测试。其中一处可能有改进的地方是在App组件中,使用CustomInput代替原本的Input组件,并向其传递props。如果想达到更好的代码复用,建议将CustomInput组件单独作为一个模块导出。此外,在onchange函数里通过状态判断是否需要重新渲染,也可以通过React.memo进行优化。对于测试部分的代码,测试对象的数量似乎比较少,可以考虑补充更多测试用例。

expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

此代码补丁主要做了以下几个事情:

  1. 导入了一些 React 组件和相关依赖。
  2. 定义了两个测试用例,测试表单的校验功能是否正常。

以下是可能存在的风险和改进建议:

  1. 可能存在性能问题。可以尝试使用 useMemo 和 useCallback 来避免重新渲染组件。
  2. 测试用例并不全面,可以增加更多场景的测试用例,以提高测试覆盖率。
  3. 在导入组件时最好按需引入,而不是直接引入整个模块。这样可以减少打包后的文件体积。

expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码主要是对 antd Form 组件进行的单元测试,包括验证表单项状态变化等。对于改进,如果有使用组件库提供的 Hooks 的话可以改成一些更简洁的方式,例如 useMountTest 和 useRtlTest;另外建议添加更多完整的 Edge Case 的测试用例。对于风险 bug,目前没有发现明显的问题。

Comment thread components/form/FormItem/ItemHolder.tsx
}, [mergedValidateStatus, hasFeedback, desplayValidateStatus]);

// ======================== Render ========================
const itemClassName = classNames(itemPrefixCls, className, rootClassName, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码是对React组件的一个补丁,主要作用是根据表单项的状态设置相应的验证状态和反馈图标。

代码使用了React的Hooks特性来管理状态,其中包括了使用useMemo钩子处理计算昂贵的对象和避免不必要的渲染。

从风险方面看,目前没有明显的漏洞或缺陷。关于改进建议,我建议可以将一些常量提取到模块级别,以便在整个模块中共享。此外,您还可以更改变量名称以更好地匹配其含义,并删除不必要的空行。

Comment thread components/form/FormItem/ItemHolder.tsx Outdated
expect(onChange).toHaveBeenNthCalledWith(idx++, 'validating');
expect(onChange).toHaveBeenNthCalledWith(idx++, 'success');
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码是关于 Ant Design 是一个 UI 组件库,主要是对其中的一个 Form 组件进行单元测试。
该代码中引用了 React 和一些 Ant Design 组件及其相关依赖。

代码中添加了一个 onChange 函数,用于在自定义组件 CustomInput 的状态改变时记录当前状态。
测试用例包括了表单验证和 validate status 状态改变的两个场景,通过异步等待 fakeTimer 的方式进行断言。测试表明表单验证和状态改变都能够成功工作。

对于代码本身,可以提供以下改进建议:

  1. 将多余的注释删除,保持代码简洁易懂;
  2. 对 import 语句进行排序和分组,方便阅读和维护;
  3. 建议加上类型声明,并定义 PropTypes;
  4. 优化重复的测试代码,避免冗余和重复的操作;

}, [mergedValidateStatus, hasFeedback, desplayValidateStatus]);

// ======================== Render ========================
const itemClassName = classNames(itemPrefixCls, className, rootClassName, {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码主要是一个antd的form组件中FormItem的渲染逻辑。在该patch中,主要加了一个displayValidateStatus常量来记录当前显示状态,并根据这个常量去渲染feedbackIcon。同时修改了一些变量名称以及优化了一些传入useMemo和useCallback Hook中的参数。

关于改进的建议,
1.建议附加注释来解释为什么这些变量需要被修改。
2.建议将变量名拼写错误的地方修复。
3.建议检查是否做好了每个文件以及引入文件的类型检查。

Comment thread components/form/FormItem/ItemHolder.tsx Outdated
`${itemPrefixCls}-feedback-icon-${desplayValidateStatus}`,
)}
>
<IconNode />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

该代码的主要作用是将一个表单元素包装在一个 FormItem 组件中,提供一些表单校验及反馈的功能。以下是我的简短代码审核:

  • 表单状态判断逻辑似乎有些冗余,建议重构为一个函数。
  • 变量名可以更改一下,以增加可读性,例如 mergedValidateStatus 可以改成 validateStatus
  • feedbackIcon 中,应该使用 validateStatus 而不是 desplayValidateStatus

关于潜在的错误风险,由于我没有看到完整的代码库和设计文档,因此无法确定是否存在问题。对于这个 FormItem 组件本身,建议注意更新依赖项并进行测试以确保稳定性和准确性,并注意防止 XSS 和 CSRF 等安全问题。对于优化建议,我们可能需要更多上下文信息才能提供建议。

`${itemPrefixCls}-feedback-icon-${desplayValidateStatus}`,
)}
>
<IconNode />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

该代码补丁主要对表单项进行了状态管理和反馈,其中包含以下改进建议:

  • getValidateState 函数中添加了参数 isDebounce,根据该参数返回不同的验证状态集合。这提高了函数复用性,并简化了判断逻辑。
  • 通过 desplayValidateStatus 变量(拼写错误,应为 displayValidateStatus)来显示实际使用的验证状态,而不是 mergedValidateStatus。这避免了副作用的影响,并减少了代码的冗余。

该代码补丁与风险问题。

Comment on lines +1632 to +1638
const CustomInput = (props: any) => {
const { status } = Form.Item.useStatus();
useEffect(() => {
onChange(status);
}, [status]);
return <Input {...props} />;
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这里是不是可以不用 useEffect,不然还要区分 react 版本

const CustomInput = (props: any) => {
    const { status } = Form.Item.useStatus();
    return <><Input {...props} />{status}</>;
};

changeValue(0, '1');
expect(status).toBe('validating')
expect(status).toBe('error')

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

主要是为了模拟用户的使用场景进行测试

`${itemPrefixCls}-feedback-icon-${desplayValidateStatus}`,
)}
>
<IconNode />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

这段代码中,没有明显的bug风险。以下是一些可能的改进建议:

  1. 减少重复代码。例如,可以将获取合并验证状态的逻辑提取为单独的函数以避免多次编写。
  2. 代码风格统一。例如,在获取合并验证状态时,有些变量名使用下划线命名法,而有些则使用驼峰命名法。建议在整个文件中保持一致的命名风格。
  3. 在getItemProps函数中,props属性的数量过多,不太易于阅读和维护。建议将props分组,将相关属性放在一起,并为每个属性添加注释来帮助理解它们的用途。
  4. 将常量值提取到单独的常量文件中。例如,"validating","error","warning"等这样的字符串应该作为常量被提出来。

以上建议旨在提高代码的可读性和可维护性。

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (6a2cad2) 100.00% compared to head (f33139f) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master    #41412   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          610       610           
  Lines        10453     10459    +6     
  Branches      2851      2854    +3     
=========================================
+ Hits         10453     10459    +6     
Impacted Files Coverage Δ
components/form/FormItem/ItemHolder.tsx 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zombieJ zombieJ merged commit bf8a36b into master Mar 23, 2023
@zombieJ zombieJ deleted the fix-validate-status branch March 23, 2023 08:40
RedJue pushed a commit to RedJue/ant-design that referenced this pull request Apr 25, 2023
…equence (ant-design#41412)

* fix: reolve FormItem's validate status trigger unexpected issue

* feat: add test case

* feat: optimize code

* feat: optimize code

* feat: optimize code

* style: format code

* git

* feat: optimize code

* feat: optimize code

* feat: optimize code

* feat: react version

* style: format code

* refactor: refactor code

* refactor: refactor code

* refactor: refactor code
RedJue pushed a commit to RedJue/ant-design that referenced this pull request Apr 25, 2023
…equence (ant-design#41412)

* fix: reolve FormItem's validate status trigger unexpected issue

* feat: add test case

* feat: optimize code

* feat: optimize code

* feat: optimize code

* style: format code

* git

* feat: optimize code

* feat: optimize code

* feat: optimize code

* feat: react version

* style: format code

* refactor: refactor code

* refactor: refactor code

* refactor: refactor code
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.

5 participants