fix(Autocomplete): isItemSelected default check is not accessible #406
Conversation
Size Changes
View raw build statsPrevious (master){
"apollo": {
"esm": 10832,
"lib": 14147
},
"app-shell": {
"esm": 12906,
"lib": 19874
},
"composer": {
"esm": 68247,
"lib": 101805
},
"core": {
"esm": 580945,
"lib": 726482
},
"forms": {
"esm": 37350,
"lib": 49298
},
"icons": {
"esm": 156355,
"lib": 205626
},
"layouts": {
"esm": 15298,
"lib": 20770
},
"metrics": {
"esm": 5467,
"lib": 7729
},
"test-utils": {
"esm": 4279,
"lib": 5937
}
}Current{
"apollo": {
"esm": 10832,
"lib": 14147
},
"app-shell": {
"esm": 12906,
"lib": 19874
},
"composer": {
"esm": 68247,
"lib": 101805
},
"core": {
"esm": 580981,
"lib": 726548
},
"forms": {
"esm": 37350,
"lib": 49298
},
"icons": {
"esm": 156355,
"lib": 205626
},
"layouts": {
"esm": 15298,
"lib": 20770
},
"metrics": {
"esm": 5467,
"lib": 7729
},
"test-utils": {
"esm": 4279,
"lib": 5937
}
} |
| @@ -643,7 +643,7 @@ export default class Autocomplete<T extends Item = Item> extends React.Component | |||
| return this.getFilteredItems(this.state).map((item: T, index) => { | |||
| const value = this.props.getItemValue!(item); | |||
| const selected = this.props.isItemSelected | |||
| ? this.props.isItemSelected(item, value) | |||
| ? this.props.isItemSelected(item, this.state.value) | |||
There was a problem hiding this comment.
@ktmud I'm not sure I agree this is a bug. it looks like the api is simply passing the item value as the second argument, not the autocomplete value.
This would be a breaking change if someone had logic that assumed the second argument was the item value.
There was a problem hiding this comment.
Currently there is not way to compare item value against state.value since isItemSelected has a default value of () => false, which you cannot override by passing undefined, the only other compatible value according to typing definition.
It would be weird (and probably not advisable) to check isItemSelected without checking it against the real selected value. Plus users shouldn't need item values, either, because they can easily get it from getItemValue.
There was a problem hiding this comment.
Plus users shouldn't need item values, either, because they can easily get it from getItemValue.
agree with this for sure
Currently there is not way to compare item
valueagainststate.value
can you give more context to when this is an issue? wouldn't you be able to get value from onChange callbacks? or in the case of a controlled component the user would be passing the value themselves?
There was a problem hiding this comment.
This function (isItemSelected) is used when rendering the menu items. I wanted to render selected values differently in the menu. Currently it's not possible to do that without passing in an isItemSelected override that depends on a value state in the parent component. If the renderItem API already passes a selected prop, then it should be correct, right?
ktmud
force-pushed
the
bugfix--autocomplete-isItemSelected
branch
from
a63db89
to
6e8b6e2
Compare
So users can use the default isItemSelected check without type warnings.
ktmud
force-pushed
the
bugfix--autocomplete-isItemSelected
branch
from
6e8b6e2
to
2bbc2ee
Compare
ktmud
changed the title
to: @williaster @alecklandgraf
Description
The doc states
isItemSelectedofAutocompletewill compare values by default, but without setting this prop, it's actually always returningfalse. Since changing the default behavior would be a breaking change, I'm addingnullto allow prop values in typing so that users can have access to the actual default behavior.Motivation and Context
See description.
Testing
Make sure
<Autocomplete isItemSelected={null} >doesn't throw type check errors.Screenshots
Checklist