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(ColorPicker): add onClear #42634

Merged
merged 4 commits into from
May 27, 2023

Conversation

linxianxi
Copy link
Contributor

@linxianxi linxianxi commented May 26, 2023

[中文版模板 / 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

#42406

💡 Background and solution

应该让用户自己控制清除时是否关闭,用 open 受控配合 onClear 实现,或者加一个类似 closePopupWhenClear 属性。

📝 Changelog

Language Changelog
🇺🇸 English add onClear and dont't close panel when click
🇨🇳 Chinese 添加 onClear,清除时不自动关闭弹窗

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

🚀 Summary

🤖 Generated by Copilot at e45c232

Added a new onClear prop to the ColorPicker component to customize the behavior when clearing the color. Refactored the color picker panel props and the popup closing and color clearing logic. Updated the documentation and the test case accordingly.

🔍 Walkthrough

🤖 Generated by Copilot at e45c232

  • Add onClear prop to ColorPickerProps interface and ColorPicker component to allow custom callback function when clearing the color (link, link, link, link, link, link, link)
  • Refactor handleClear function in ColorPicker component to set colorCleared state and call onClear prop function without passing boolean parameter (link)
  • Remove useEffect hook and import from ColorPicker component as popup closing logic is moved to handleClear function (link, link)
  • Change onClear prop in ColorPickerPanelProps interface and ColorPickerPanel component to a function without parameters to match ColorPickerProps and handleClear function (link, link)
  • Move onFormatChange prop from ColorPickerProps to ColorPickerPanelProps as it is more relevant for customizing color format (link, link, link, link)
  • Move onChange and onOpenChange props from ColorPickerProps to ColorPickerPanelProps as they are more relevant for customizing color picker panel behavior (link, link)
  • Remove assertion that popup should be hidden after clearing color from test case in components/color-picker/__tests__/index.test.tsx as it is handled by handleClear function (link)

@github-actions
Copy link
Contributor

github-actions bot commented May 26, 2023

@kiner-tang kiner-tang requested review from MadCcc and RedJue May 26, 2023 03:06
@RedJue
Copy link
Member

RedJue commented May 26, 2023

补个 onClear 的单测,allowClear demo 可以完善一下,加个 onClear 控制 panel 关闭的例子。

@linxianxi linxianxi changed the title feat(ColorPicker): add onClear and dont't close panel when click feat(ColorPicker): add onClear and dont't close panel when clear May 26, 2023
@RedJue
Copy link
Member

RedJue commented May 26, 2023

单测挂了,本地调试一下。

@linxianxi
Copy link
Contributor Author

linxianxi commented May 26, 2023

@RedJue 这个单测不知道怎么不行,你看看

@linxianxi
Copy link
Contributor Author

linxianxi commented May 26, 2023

还有个问题,一直点击清除,会一直触发 onChange 和 onClear,0 和 100 会反复切换,这是故意为之还是 bug?已经是 clear 状态再点清除不是应该什么都不发生吗

@RedJue
Copy link
Member

RedJue commented May 26, 2023

还有个问题,一直点击清除,会一直触发 onChange 和 onClear,0 和 100 会反复切换,这是故意为之还是 bug?已经是 clear 状态再点清除不是应该什么都不发生吗

bug 如果清空状态下不应该被点击,这个可以单独提个 fix修复。

@kiner-tang
Copy link
Member

还有个问题,一直点击清除,会一直触发 onChange 和 onClear,0 和 100 会反复切换,这是故意为之还是 bug?已经是 clear 状态再点清除不是应该什么都不发生吗

bug 如果清空状态下不应该被点击,这个可以单独提个 fix修复。

这个可以跟 清空后不隐藏面板 的问题一起修复,然后提 pr 到 master

fireEvent.click(container.querySelector('.ant-color-picker-trigger')!);
await waitFakeTimer();
expect(container.querySelector('.ant-popover-hidden')).toBeFalsy();
Copy link
Member

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.

加上了之后,最后那行还是 0%,得不到 100%

Copy link
Member

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.

我知道不一样,删掉那几行是可以了,但是这几行看上去感觉没问题。

Copy link
Member

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.

这几行意思是点击一下方块,获取值是 0,应该没问题吧,跟是否关闭弹窗有关系么。删了删了,等另一个 pr 合并再更新这里的。

@RedJue
Copy link
Member

RedJue commented May 26, 2023

@RedJue 这个单测不知道怎么不行,你看看

猜测可能单测里行为和现在的预期不一样,尝试删掉这几行 https://github.com/ant-design/ant-design/pull/42634/files#diff-aa06460351e8630f305cf6509e8137984e0f167ab2560da22874033189234fb4L99-L103 试试

@linxianxi linxianxi changed the title feat(ColorPicker): add onClear and dont't close panel when clear feat(ColorPicker): add onClear May 26, 2023
@linxianxi linxianxi requested a review from RedJue May 26, 2023 07:20
@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2777f26) 100.00% compared to head (cf24e3d) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##           feature    #42634   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          641       641           
  Lines        10891     10889    -2     
  Branches      2958      2957    -1     
=========================================
- Hits         10891     10889    -2     
Impacted Files Coverage Δ
components/color-picker/ColorPicker.tsx 100.00% <100.00%> (ø)
components/color-picker/ColorPickerPanel.tsx 100.00% <100.00%> (ø)

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

@@ -1,7 +1,7 @@
## zh-CN

清除已选择的颜色。
清除已选择的颜色。可以使用受控 open 和 onClear 控制清除时关闭弹窗。
Copy link
Member

Choose a reason for hiding this comment

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

demo 分两个吧,一个最基础的用法,一个配合 onClear 实现弹窗关闭。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

其实我觉得就只要最基础的用法就好了,配合 onClear 关闭没有必要。

Copy link
Member

Choose a reason for hiding this comment

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

不加应该问题也不大,先这样吧。

@linxianxi linxianxi requested a review from RedJue May 26, 2023 08:33
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.

3 participants