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: improve key type to support React.Key #692

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madocto
Copy link
Contributor

@madocto madocto commented Mar 19, 2024

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (d21b6bb) to head (e7abb7e).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #692   +/-   ##
=======================================
  Coverage   99.58%   99.58%           
=======================================
  Files          27       27           
  Lines         721      721           
  Branches      196      198    +2     
=======================================
  Hits          718      718           
  Misses          3        3           

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

@@ -6,14 +6,16 @@ interface ItemSharedProps {
className?: string;
}

export type MenuKey = React.Key;
Copy link
Contributor

Choose a reason for hiding this comment

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

这个好不容易是 string 了,又要改会 key 吗,如果要改,改成 T 吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

React.Key 会有问题吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

里面包含 number,如果我想执行处理字符串的方法,还需要转一次字符串

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还是希望 string | number 都能支持,并且输入的类型和输出的类型一致,如果合并了 #692 ,那 React.Key 默认是不是就支持了?

Copy link
Contributor

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.

范型 +1,React.Key 有时会包含奇怪的东西,让用户自己指定 key 类型比较好

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

3 participants