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: Modal footer support custom render function #44318

Merged
merged 9 commits into from Aug 28, 2023
Merged

Conversation

RedJue
Copy link
Member

@RedJue RedJue commented Aug 21, 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

close #44297

💡 Background and solution

image
image

📝 Changelog

Language Changelog
🇺🇸 English Modal footer support custom render function
🇨🇳 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

🚀 Summary

🤖 Generated by Copilot at 641db8d

This pull request adds a new feature to the Modal component and its methods, allowing users to customize the footer rendering function with the footer prop. It also refactors the footer buttons into separate components and uses context to pass the props. It updates the documentation, the type definitions, and the test cases accordingly.

🔍 Walkthrough

🤖 Generated by Copilot at 641db8d

  • Add custom footer rendering function feature to Modal and Modal.confirm methods (link, link, link, link, link, link, link, link, link)
  • Use ConfirmCancelBtn and ConfirmOkBtn components to replace ActionButton in ConfirmContent function (link, link, link)
  • Use NormalCancelBtn and NormalOkBtn components to replace Button in Footer function (link, link, link)
  • Use context and context providers to pass props to footer buttons (link, link, link)
  • Support footer prop as a function that receives origin node and extra components as arguments (link, link, link, link, link, link, link, link, link, link)
  • Add examples and documentation for the feature in both Chinese and English (link, link, link, link)
  • Add test cases to check custom footer rendering function feature in Modal and Modal.confirm methods (link, link)
  • Remove unused import of ActionButton from ConfirmDialog (link)

@stackblitz
Copy link

stackblitz bot commented Aug 21, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@RedJue RedJue requested a review from zombieJ August 21, 2023 10:07
@github-actions
Copy link
Contributor

github-actions bot commented Aug 21, 2023

@@ -54,7 +55,7 @@ demo:
| confirmLoading | 确定按钮 loading | boolean | false | |
| destroyOnClose | 关闭时销毁 Modal 里的子元素 | boolean | false | |
| focusTriggerAfterClose | 对话框关闭后是否需要聚焦触发元素 | boolean | true | 4.9.0 |
| footer | 底部内容,当不需要默认底部按钮时,可以设为 `footer={null}` | ReactNode | (确定取消按钮) | |
| footer | 底部内容,当不需要默认底部按钮时,可以设为 `footer={null}` | (originNode: React.ReactNode, extra: { ConfirmBtn: FC; CancelBtn: FC }) => React.ReactNode \| React.ReactNode | (确定取消按钮) | 5.9.0 |
Copy link
Member

Choose a reason for hiding this comment

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

感觉需要有个 TS 定义放在 Popover 的交互比较好,这里面越写越长了。

Copy link
Member Author

Choose a reason for hiding this comment

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

可以,我加一个类型吧,然后链接过去

@github-actions
Copy link
Contributor

github-actions bot commented Aug 21, 2023

size-limit report 📦

Path Size
./dist/antd.min.js 390.73 KB (+396 B 🔺)
./dist/antd-with-locales.min.js 449.65 KB (+415 B 🔺)

@argos-ci
Copy link

argos-ci bot commented Aug 21, 2023

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 🧿 Changes detected (Review) 16 changes Aug 28, 2023, 3:17 AM

@afc163
Copy link
Member

afc163 commented Aug 21, 2023

体积怎么多了这么多。

@RedJue
Copy link
Member Author

RedJue commented Aug 21, 2023

体积怎么多了这么多。

代码层面感觉加的不多,是第三方依赖更新?

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (68ee322) 100.00% compared to head (134c935) 100.00%.
Report is 11 commits behind head on feature.

Additional details and impacted files
@@            Coverage Diff            @@
##           feature    #44318   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          663       668    +5     
  Lines        11309     11325   +16     
  Branches      3051      3062   +11     
=========================================
+ Hits         11309     11325   +16     
Files Changed Coverage Δ
components/date-picker/style/index.ts 100.00% <ø> (ø)
components/dropdown/dropdown.tsx 100.00% <ø> (ø)
components/input/style/index.ts 100.00% <ø> (ø)
components/tour/style/index.ts 100.00% <ø> (ø)
components/form/style/index.ts 100.00% <100.00%> (ø)
components/message/index.tsx 100.00% <100.00%> (ø)
components/modal/ConfirmDialog.tsx 100.00% <100.00%> (ø)
components/modal/Modal.tsx 100.00% <100.00%> (ø)
components/modal/components/ConfirmCancelBtn.tsx 100.00% <100.00%> (ø)
components/modal/components/ConfirmOkBtn.tsx 100.00% <100.00%> (ø)
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@afc163
Copy link
Member

afc163 commented Aug 21, 2023

不是的,这个体积对比就只对比代码本身的变化。看上去拆了很多新组件文件出来,看下是否有必要。

@RedJue
Copy link
Member Author

RedJue commented Aug 21, 2023

不是的,这个体积对比就只对比代码本身的变化。看上去拆了很多新组件文件出来,看下是否有必要。

因为这个 API 需要把原来写在一起的 Button 抽离成单个组件,好像也没法避免?

close,
onCancel,
onConfirm,
};
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.

我尝试去掉吧,再优化一下

rootPrefixCls,
close,
onCancel,
onConfirm,
Copy link
Member

Choose a reason for hiding this comment

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

这里也有一排

Comment on lines 97 to 101
footer === undefined ? <Footer {...props} onOk={handleOk} onCancel={handleCancel} /> : footer;
footer === undefined || typeof footer === 'function' ? (
<Footer {...props} onOk={handleOk} onCancel={handleCancel} />
) : (
footer
);
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.

逻辑感觉不是很复杂吧,一个三目感觉可以搞定?

Comment on lines 128 to 156
{footer === undefined ? (
<div className={`${confirmPrefixCls}-btns`}>
{cancelButton}
<ActionButton
isSilent={isSilent}
type={okType}
actionFn={onOk}
close={(...args: any[]) => {
close?.(...args);
onConfirm?.(true);
}}
autoFocus={autoFocusButton === 'ok'}
buttonProps={okButtonProps}
prefixCls={`${rootPrefixCls}-btn`}
>
{okText || (mergedOkCancel ? mergedLocale?.okText : mergedLocale?.justOkText)}
</ActionButton>
</div>

{footer === undefined || typeof footer === 'function' ? (
<ConfirmOkBtnProvider value={confirmBtnCtxValueMemo}>
<ConfirmCancelBtnContextProvider value={cancelBtnCtxValueMemo}>
<div className={`${confirmPrefixCls}-btns`}>
{footer === undefined
? footerOriginNode
: footer?.(footerOriginNode, {
ConfirmBtn,
CancelBtn,
})}
</div>
</ConfirmCancelBtnContextProvider>
</ConfirmOkBtnProvider>
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.

确实,可能嵌套多了,看起来代码不是很直观,但这也是我目前能想到最简洁的了,有优化建议不?

Comment on lines 184 to 191
## footerRenderParams

<!-- prettier-ignore -->
| Property | Description | Type | Default |
| --- | --- | --- | --- |
| originNode | default node | React.ReactNode | - |
| extra | extended options | { ConfirmBtn: FC; CancelBtn: FC } | - |

Copy link
Member

Choose a reason for hiding this comment

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

感觉这里可以直接用 tsx + interface , 比如:(伪代码)

type FooterBuiltinProps = {
  ConfirmBtn: React.ReactNode;
  CancelBtn: React.ReactNode;
}
type RenderFooter = (originNode: FooterProps, extra: FooterBuiltinProps) => React.ReactNode;

export type FooterProps = React.ReactNode | RenderFooter;

Copy link
Member Author

Choose a reason for hiding this comment

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

文档里好像很难显示ts定义

Copy link
Member

Choose a reason for hiding this comment

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

那就先保持你的写法~

Comment on lines 185 to 192
## footerRenderParams

<!-- prettier-ignore -->
| 参数 | 说明 | 类型 | 默认值 |
| --- | --- | --- | --- |
| originNode | 默认节点 | React.ReactNode | - |
| extra | 扩展选项 | { ConfirmBtn: FC; CancelBtn: FC } | - |

Copy link
Member

Choose a reason for hiding this comment

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

同上👆🏻

<NormalCancelBtn />
<NormalOkBtn />
</>
);
Copy link
Member

@afc163 afc163 Aug 23, 2023

Choose a reason for hiding this comment

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

这段代码上面出现过一次?和 components/modal/ConfirmDialog.tsx 里的改动基本一样。

Copy link
Member Author

Choose a reason for hiding this comment

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

大体逻辑差不多,正常的 ModalModal.confirm 两边按钮逻辑不太一样,所以又拆了两个按钮出来

components/modal/shared.tsx Outdated Show resolved Hide resolved

const confirmBtnCtxValueMemo = React.useMemo(
() => confirmBtnCtxValue,
[...Object.values(confirmBtnCtxValue)],
Copy link
Member

Choose a reason for hiding this comment

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

这样写行不行呢?(其实我也不太熟悉 23333~)

Suggested change
[...Object.values(confirmBtnCtxValue)],
Object.values(confirmBtnCtxValue),

Copy link
Member 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.

你先本地试试, confirmBtnCtxValue如果是 undefined values 是不是会报错

Copy link
Member Author

Choose a reason for hiding this comment

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

这是自己定义的,不会是 undefined

);
const cancelBtnCtxValueMemo = React.useMemo(
() => cancelBtnCtxValue,
[...Object.values(cancelBtnCtxValue)],
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
[...Object.values(cancelBtnCtxValue)],
Object.values(cancelBtnCtxValue),

Comment on lines 184 to 191
## footerRenderParams

<!-- prettier-ignore -->
| Property | Description | Type | Default |
| --- | --- | --- | --- |
| originNode | default node | React.ReactNode | - |
| extra | extended options | { ConfirmBtn: FC; CancelBtn: FC } | - |

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 feat-modal-footer branch 2 times, most recently from c39e959 to 01ff711 Compare August 24, 2023 03:32
Comment on lines 8 to 13
export const NormalCancelBtnContext = React.createContext<NormalCancelBtnProps>(
{} as NormalCancelBtnProps,
);
export const NormalOkBtnContext = React.createContext<NormalOkBtnProps>({} as NormalOkBtnProps);
export const ConfirmCancelBtnContext = React.createContext<ConfirmCancelBtnProps>(
{} as ConfirmCancelBtnProps,
Copy link
Member

Choose a reason for hiding this comment

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

这里能不能只用一个 context,本身都是 Modal 的内部属性

Copy link
Member Author

Choose a reason for hiding this comment

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

应该可以收敛成一个,我一会优化一下

@@ -42,23 +45,46 @@ export const Footer: React.FC<
onCancel,
okButtonProps,
cancelButtonProps,
footer,
Copy link
Member

Choose a reason for hiding this comment

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

感觉不应该把 footer 放到 Footer 组件里处理,逻辑应该和 node 一样是整体替换掉的

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.

那我都收到里面去统一处理

@afc163
Copy link
Member

afc163 commented Aug 25, 2023

多了更多了,加这个功能多了 400B ..

@RedJue
Copy link
Member Author

RedJue commented Aug 25, 2023

多了更多了,加这个功能多了 400B ..

要拆组件出来好像也确实避免不了...😅

</DisabledContextProvider>
) : (
footer
Copy link
Member

Choose a reason for hiding this comment

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

这个是不是也应该放在 provider 里?

Copy link
Member Author

Choose a reason for hiding this comment

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

应该不用吧,这个footer应该是用户自己定义的节点,不应该受 provider 的影响

@RedJue RedJue merged commit e7c7601 into feature Aug 28, 2023
86 of 87 checks passed
@RedJue RedJue deleted the feat-modal-footer branch August 28, 2023 03:54
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