Fix bug in combo box where _selectedElement ref wasn't being set conditionally#4350
Conversation
|
Consider adding a unit test to help catch if the ref gets screwed up in the future |
jspurlin
left a comment
jspurlin
left a comment
There was a problem hiding this comment.
consider adding a unit test
|
@kelseyyoung did you find the merge that introduced this regression? If so, can you list it here for reference? |
|
After discussion with @jspurlin it seems like there's no good way to test it reliably |
| ariaLabel= {item.text } | ||
| disabled={ item.disabled } | ||
| > { <span ref={ this._selectedElement }> | ||
| > { <span ref={ isSelected ? this._selectedElement : '' }> |
There was a problem hiding this comment.
Since we're trying to move away from string refs perhaps pass a noop?
const noop = () => {};
...
> { <span ref={ isSelected ? this._selectedElement : noop }>There was a problem hiding this comment.
What about undefined? TSLint won't pass with an empty function block
There was a problem hiding this comment.
Yeah, I think undefined would be fine, you may need to update the declaration to allow for undefined
There was a problem hiding this comment.
I think it's fine as is, ref is already an optional prop and passing undefined should just make it not set to anything. I'll push that in a minute
3 participants
Pull request checklist
$ npm run changeDescription of changes
There was a large change refs recently, and during that change a conditional ref assignment was omitted. The _selectedElement ref in ComboBox was always being set instead of only being set to the actual selected element. This meant the ComboBox Callout always scrolled to the bottom. This fix puts back that conditional ref assignment
PR of the original ref change: #4303
Focus areas to test
Tested in Fabric demo