-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
fix(select): fix typing for customLabel #18952
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18952 +/- ##
==========================================
- Coverage 66.39% 66.37% -0.02%
==========================================
Files 1641 1641
Lines 63503 63518 +15
Branches 6418 6422 +4
==========================================
+ Hits 42162 42163 +1
- Misses 19681 19694 +13
- Partials 1660 1661 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
||
export type RawValues = RawValue[]; | ||
export type LabeledValues = LabeledValue[]; | ||
export type SelectValue = RawValue | LabeledValue | RawValues | LabeledValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but do you think we could make SelectValue
internal instead of exporting it? I can see the use cases for the other types but I think when we are using a Select we already defined if their values will be RawValue
or LabeledValue
. Considering that we are not mixing the types for the same Select.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with either way. But ideally the Select
component itself should probably be generic. Similar to this: https://github.com/ktmud/chakra-ui-select/blob/dee16e260ba4bd64cfead81cf6ab4cf2af12877f/Select/Select.tsx#L75-L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of generics as you mentioned. Maybe it can be a future improvement.
export interface LabeledValue { | ||
key?: string; | ||
value: RawValue; | ||
label: ReactNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea when introducing the customLabel
prop was that label
would always be a string
that could be searched/compared and customLabel
would be used when you want a React.Node
. The hasOption
function is comparing the labels as strings. One example of this use is the tables select where label is the name of the table, customLabel
is a node with an icon and the name, and value is the id
of the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the intention but changing it to ReactNode
will cause too many incompatibility in other references of the Select
component. Ideally we should probably refactor customLabel
to something akin to optionalRenderer
. It also doesn't feel right for a data fetching API (the options
promise) to return ReactNode
. Data fetching and rendering should be two separate things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the improvements @ktmud! I left a couple of comments.
Closing for not in favor of refactor out |
SUMMARY
Typing for
option.customLabel
in the Select component is not correctly applied because it extended the wrong source type. This should fix it. No changes to code functionalities whatsoever.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
.customLabel
isany
where it was referenced:After
.customLabel
is ReactNodeTESTING INSTRUCTIONS
CI
ADDITIONAL INFORMATION