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 implement showText API #42865

Merged
merged 13 commits into from Jun 19, 2023
Merged

Conversation

RedJue
Copy link
Member

@RedJue RedJue commented Jun 6, 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

💡 Background and solution

Light:

image

Dark:

image

📝 Changelog

Language Changelog
🇺🇸 English ColorPicker implement showText API
🇨🇳 Chinese 颜色选择器 实现 showText API

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

This pull request adds a new feature to the ColorPicker component that allows showing the selected color as a text value in different formats. It updates the component logic, style, documentation, and tests to support this feature. It also adds a new demo file showValue.tsx and its corresponding markdown file showValue.md to showcase the feature.

🔍 Walkthrough

🤖 Generated by Copilot at 2babb91

  • Add a new prop showValue to the ColorPicker component, which controls whether to show the selected color as a string in the trigger component (link, link, link, link, link).

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Pull reviewers stats

Stats of the last 30 days for ant-design:

User Total reviews Time to review Total comments
afc163 93 1h 54m 80
li-jia-nan 90 2h 19m 79
MadCcc 55 1d 22h 44m 122
kiner-tang 51 20m 30
yoyo837 33 43m 33
zombieJ 30 2h 32m 25
RedJue 10 3h 50m 15
heiyu4585 2 30m 0
RexSkz 2 48m 5
PeachScript 1 29m 0
vagusX 1 1h 36m 0
Wxh16144 1 21m 3
any1024 1 7h 10m 2
sawadyecma 1 1h 51m 2
bombillazo 1 23h 49m 1
BoyYangzai 1 20m 2
lke-twh 1 2h 49m 0

@RedJue RedJue requested a review from MadCcc June 6, 2023 11:36
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

size-limit report 📦

Path Size
./dist/antd.min.js 377.02 KB (+202 B 🔺)
./dist/antd-with-locales.min.js 436.47 KB (+231 B 🔺)

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2c20dcd) 100.00% compared to head (0372c2f) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##           feature    #42865   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          650       650           
  Lines        10956     10974   +18     
  Branches      2971      2979    +8     
=========================================
+ Hits         10956     10974   +18     
Impacted Files Coverage Δ
components/color-picker/style/index.ts 100.00% <ø> (ø)
components/color-picker/ColorPicker.tsx 100.00% <100.00%> (ø)
...omponents/color-picker/components/ColorTrigger.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.

| disabled | 禁用颜色选择器 | boolean | - | |
| placement | 弹出窗口的位置 | `top` \| `topLeft` \| `topRight` \| `bottom` \| `bottomLeft` \| `bottomRight` | `bottomLeft` | |
| arrow | 配置弹出的箭头 | `boolean \| { pointAtCenter: boolean }` | true | |
| showValue | 是否显示选定的颜色 | `boolean` | - | 5.7.0 |
Copy link
Member

Choose a reason for hiding this comment

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

这个 showValue 直观上看不出来是啥意思,是不是类似 DatePicker 一样给个 format 函数?

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
Member Author

Choose a reason for hiding this comment

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

目前只是简单的根据 format 生成对应的颜色字符串,如果需要深度定制 format 格式确实需要一个函数,意下如何? cc @MadCcc

Copy link
Member

Choose a reason for hiding this comment

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

函数吧,一步到位。

@@ -39,6 +39,7 @@ export interface ColorPickerProps
allowClear?: boolean;
presets?: PresetsItem[];
arrow?: boolean | { pointAtCenter: boolean };
showValue?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

我意思是不需要 showValue 这个属性了,这个命名不好,不容易理解。

直接用 format 取代掉。

Copy link
Member Author

Choose a reason for hiding this comment

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

format 已经有对应属性了,换一个?

Copy link
Member

Choose a reason for hiding this comment

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

确实,<ColorPicker inputRender={} /> 好了。

Copy link
Member Author

@RedJue RedJue Jun 7, 2023

Choose a reason for hiding this comment

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

inputRender:( formatStr:string ) => string
or
inputRender:( formatStr:string, color:Color) => string ? @afc163

Copy link
Member

@afc163 afc163 Jun 7, 2023

Choose a reason for hiding this comment

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

inputRender: (color: Color) => React.ReactNode

Copy link
Member Author

Choose a reason for hiding this comment

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

color 是对象还是字符串

Copy link
Member

Choose a reason for hiding this comment

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

对象

@RedJue RedJue force-pushed the feature-color-picker-showvalue branch from bb68bbd to 32483ae Compare June 19, 2023 02:05
@RedJue RedJue changed the title feat: ColorPicker implement showValue API feat: ColorPicker implement textRender API Jun 19, 2023
@RedJue RedJue changed the title feat: ColorPicker implement textRender API feat: ColorPicker implement textRender API Jun 19, 2023
@@ -40,6 +42,9 @@ const ColorTrigger = forwardRef<HTMLDivElement, colorTriggerProps>((props, ref)
{...rest}
>
{containerNode}
{typeof textRender === 'function' && (
Copy link
Member

Choose a reason for hiding this comment

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

这个在 ts 中已经约束了函数,这里不需要判断类型了吧

Suggested change
{typeof textRender === 'function' && (
{textRender && (

Copy link
Member

Choose a reason for hiding this comment

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

兜底也可以,可以保证 js 不会报错

Copy link
Member

Choose a reason for hiding this comment

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

我的想法是这种情况反而不需要兜底,如果用户传了不是函数,就让 js 把报错抛出去,兜底的话用户反而看不到错误,不知道为啥这里不显示 dom

Copy link
Member Author

Choose a reason for hiding this comment

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

如果值隐式转换成boolean ,可能会多一个空节点。

Co-authored-by: MadCcc <1075746765@qq.com>
alignItems: 'center',
justifyContent: 'center',
transition: `all ${motionDurationMid}`,
background: colorBgElevated,
padding: paddingXXS - 1,
Copy link
Member

Choose a reason for hiding this comment

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

1 可以写成 lineWidth

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -142,6 +146,12 @@ const genColorPickerStyle: GenerateStyle<ColorPickerToken> = (token) => {
borderColor: colorBgTextActive,
},
},
[`${componentCls}-trigger-text`]: {
marginInlineStart: marginXS,
marginInlineEnd: marginXS - 2,
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
Member Author

Choose a reason for hiding this comment

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

这里改成 lineWidth * 2 ?

@fuji221
Copy link

fuji221 commented Jun 19, 2023

image
文字颜色如上

@@ -142,6 +146,12 @@ const genColorPickerStyle: GenerateStyle<ColorPickerToken> = (token) => {
borderColor: colorBgTextActive,
},
},
[`${componentCls}-trigger-text`]: {
marginInlineStart: marginXS,
marginInlineEnd: marginXS - lineWidth * 2,
Copy link
Member

Choose a reason for hiding this comment

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

这里的 lineWidth*2 没什么道理,如果是两边对称应该是减去原有的 padding

@@ -40,6 +42,9 @@ const ColorTrigger = forwardRef<HTMLDivElement, colorTriggerProps>((props, ref)
{...rest}
>
{containerNode}
{typeof textRender === 'function' && (
<div className={`${colorTriggerPrefixCls}-text`}>{textRender(color)}</div>
Copy link
Member

Choose a reason for hiding this comment

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

感觉还是加个默认的 showText 好点,textRender 还是要写代码

Copy link
Member Author

Choose a reason for hiding this comment

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

showText 跟之前 showValue 是一个意思?

@RedJue RedJue changed the title feat: ColorPicker implement textRender API feat: ColorPicker implement showText API Jun 19, 2023
@MadCcc MadCcc merged commit 1fb8fc5 into feature Jun 19, 2023
92 of 93 checks passed
@MadCcc MadCcc deleted the feature-color-picker-showvalue branch June 19, 2023 11:29
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

5 participants