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: ⌨️ Popconfirm can be closed by pressing ESC #24420

Merged
merged 8 commits into from
May 25, 2020

Conversation

afc163
Copy link
Member

@afc163 afc163 commented May 23, 2020

[中文版模板 / Chinese template]

🤔 This is a ...

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

🔗 Related issue link

💡 Background and solution

close #15203

📝 Changelog

Language Changelog
🇺🇸 English Popconfirm can be closed by pressing ESC now.
🇨🇳 Chinese Popconfirm 支持按 ESC 关闭。

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • 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

@afc163 afc163 requested a review from zombieJ as a code owner May 23, 2020 11:57
Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Could you please add tests to make sure this change works as expected?

@afc163 afc163 changed the base branch from master to feature May 23, 2020 11:58
@ant-design-bot
Copy link
Contributor

ant-design-bot commented May 23, 2020

Comment on lines 94 to 86
const onKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
if (e.key === 'Escape' && e.target === innerContentRef.current) {
settingVisible(false, e);
}
Copy link

Choose a reason for hiding this comment

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

还要判断一下 e.target === innerContentRef.current ,学习了 😂

Comment on lines 54 to 63
React.useEffect(() => {
const timeout = setTimeout(() => {
if (visible) {
innerContentRef.current?.focus();
}
});
return () => {
clearTimeout(timeout);
};
}, [visible]);
Copy link

Choose a reason for hiding this comment

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

如果在 Popconfirm 受控的情况下,切换了 focus 就不能按 Esc 退出了吧?

找了下默认是怎么做到在外部点击关闭弹窗的,应该是这里 。那么即使在受控的情况下,也不用自己监听外部点击事件了,配置 onVisibleChange 就行了啊。之前还琢磨怎么自己监听实现呢 _(:3J∠)_

@codecov
Copy link

codecov bot commented May 23, 2020

Codecov Report

Merging #24420 into feature will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           feature   #24420   +/-   ##
========================================
  Coverage    98.95%   98.95%           
========================================
  Files          364      364           
  Lines         7391     7398    +7     
  Branches      2000     2054   +54     
========================================
+ Hits          7314     7321    +7     
  Misses          77       77           
Impacted Files Coverage Δ
components/popconfirm/index.tsx 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b485d3...1413693. Read the comment docs.

React.useEffect(() => {
const timeout = setTimeout(() => {
if (visible) {
innerContentRef.current?.focus();
Copy link
Member

Choose a reason for hiding this comment

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

这样键盘交互焦点就丢了,是不是可以考虑全局监听一下。打开的时候 ESC 关闭?

Copy link
Member Author

Choose a reason for hiding this comment

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

直接绑定在 trigger 上好了,全局监听就复杂了。

@afc163 afc163 mentioned this pull request May 23, 2020
13 tasks
@@ -72,6 +78,12 @@ const Popconfirm = React.forwardRef<unknown, PopconfirmProps>((props, ref) => {
}
};

const onKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
if (e.key === 'Escape' && visible) {
Copy link
Member

Choose a reason for hiding this comment

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

KeyCode.ESC

@afc163
Copy link
Member Author

afc163 commented May 25, 2020

/rebase

@github-actions github-actions bot force-pushed the feat-popconfirm-esc branch from 27e4935 to 1413693 Compare May 25, 2020 03:51
@afc163
Copy link
Member Author

afc163 commented May 25, 2020

超了 0.1 kB..

@afc163
Copy link
Member Author

afc163 commented May 25, 2020

Need to approve

@afc163 afc163 merged commit c93400d into feature May 25, 2020
@afc163 afc163 deleted the feat-popconfirm-esc branch May 25, 2020 09:44
@zombieJ zombieJ mentioned this pull request May 31, 2020
13 tasks
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.

Closing Popconfirm using esc key
5 participants