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.

Sign up for GitHub

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

Jump to bottom

fix(Modal): fix confirm icon style when using third-party icons #48490

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(Modal): fix confirm icon style when using third-party icons #48490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0278c79
fix(Modal): fix confirm icon style when using third-party icons
guoyunhe Apr 16, 2024
8489d22
Merge branch 'master' into fix-modal-icon-color
afc163 Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/modal/ConfirmDialog.tsx
View file
Open in desktop
Expand Up @@ -142,7 +142,7 @@ export function ConfirmContent(
[`${bodyCls}-has-title`]: hasTitle,
})}
>
{mergedIcon}
<div className={`${confirmPrefixCls}-icon`}>{mergedIcon}</div>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<div className={`${confirmPrefixCls}-icon`}>{mergedIcon}</div>
{cloneElement(mergedIcon, {
className: 'anticon',
})}

直接这样改如何？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afc163 这样会覆盖掉原本的 icon 的 className 吧，而且也有可能组件不支持外部传入 className

Copy link
Member

Choose a reason for hiding this comment

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

  1. 我试了一下不会覆盖。
  2. 常用的图标组件都会支持 className 的，这个应该是标配。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

效果相同的情况下，感觉使用 cloneElement 会更慢。

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.

任何 clone 的过程都是耗时的。而且 React 官方文档里并不推荐用 cloneElement https://react.dev/reference/react/cloneElement

Using cloneElement is uncommon and can lead to fragile code.

<div className={`${confirmPrefixCls}-paragraph`}>
{hasTitle && <span className={`${confirmPrefixCls}-title`}>{props.title}</span>}
<div className={`${confirmPrefixCls}-content`}>{props.content}</div>
Expand Down
22 changes: 12 additions & 10 deletions components/modal/style/confirm.ts
View file
Open in desktop
Expand Up @@ -44,7 +44,8 @@ const genModalConfirmStyle: GenerateStyle<ModalToken> = (token) => {
flexWrap: 'nowrap',
alignItems: 'start',

[`> ${token.iconCls}`]: {
[`> ${confirmComponentCls}-icon`]: {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[`> ${confirmComponentCls}-icon`]: {
[`> ${token.iconCls}, > svg`]: {

这样吧。

Copy link
Member

Choose a reason for hiding this comment

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

我还是想尽量不动 dom 结构。

display: 'flex',
flex: 'none',
fontSize: modalConfirmIconSize,
marginInlineEnd: token.confirmIconMarginInlineEnd,
Expand All @@ -53,7 +54,7 @@ const genModalConfirmStyle: GenerateStyle<ModalToken> = (token) => {
.div(2)
.equal(),
},
[`&-has-title > ${token.iconCls}`]: {
[`&-has-title > ${confirmComponentCls}-icon`]: {
marginTop: token
.calc(token.calc(modalTitleHeight).sub(modalConfirmIconSize).equal())
.div(2)
Expand All @@ -69,7 +70,7 @@ const genModalConfirmStyle: GenerateStyle<ModalToken> = (token) => {
},

// https://github.com/ant-design/ant-design/issues/48159
[`${token.iconCls} + ${confirmComponentCls}-paragraph`]: {
[`${confirmComponentCls}-icon + ${confirmComponentCls}-paragraph`]: {
maxWidth: `calc(100% - ${unit(
token.calc(token.modalConfirmIconSize).add(token.marginSM).equal(),
)})`,
Expand Down Expand Up @@ -100,20 +101,21 @@ const genModalConfirmStyle: GenerateStyle<ModalToken> = (token) => {
},
},

[`${confirmComponentCls}-error ${confirmComponentCls}-body > ${token.iconCls}`]: {
[`${confirmComponentCls}-error ${confirmComponentCls}-body > ${confirmComponentCls}-icon`]: {
color: token.colorError,
},

[`${confirmComponentCls}-warning ${confirmComponentCls}-body > ${token.iconCls},
${confirmComponentCls}-confirm ${confirmComponentCls}-body > ${token.iconCls}`]: {
color: token.colorWarning,
},
[`${confirmComponentCls}-warning ${confirmComponentCls}-body > ${confirmComponentCls}-icon,
${confirmComponentCls}-confirm ${confirmComponentCls}-body > ${confirmComponentCls}-icon`]:
{
color: token.colorWarning,
},

[`${confirmComponentCls}-info ${confirmComponentCls}-body > ${token.iconCls}`]: {
[`${confirmComponentCls}-info ${confirmComponentCls}-body > ${confirmComponentCls}-icon`]: {
color: token.colorInfo,
},

[`${confirmComponentCls}-success ${confirmComponentCls}-body > ${token.iconCls}`]: {
[`${confirmComponentCls}-success ${confirmComponentCls}-body > ${confirmComponentCls}-icon`]: {
color: token.colorSuccess,
},
};
Expand Down