fix(Select): improve type consistency in onSelect#11935
fix(Select): improve type consistency in onSelect#11935kmcfaul merged 2 commits intopatternfly:mainfrom
onSelect#11935Conversation
|
Preview: https://patternfly-react-pr-11935.surge.sh A11y report: https://patternfly-react-pr-11935-a11y.surge.sh Preview: https://pf-react-pr-11935.surge.sh A11y report: https://pf-react-pr-11935-a11y.surge.sh |
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
Would this be a breaking change? If consumers are expecting that their onSelect callback will be only a string or number and have logic/typing set up to only handle those types would this result in errors?
Since this is loosening a type, nothing should break as users of the callback can continue to use only strings or numbers as before |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Could you write a couple unit tests showing that string, number, etc will all work?
| shouldFocusFirstItemOnOpen?: boolean; | ||
| /** Function callback when user selects an option. */ | ||
| onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: string | number) => void; | ||
| onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, value?: SelectOptionProps['value']) => void; |
There was a problem hiding this comment.
Menu's onSelect should also be updated to reference MenuItem's itemId.
logonoff
force-pushed
the
selectoption
branch
2 times, most recently
from
c05fc3e to
599d525
Compare
logonoff
force-pushed
the
selectoption
branch
from
b1e15da to
64fe39f
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
6 participants
What: Closes #11361
Non-breaking change to loosen the expected type of
onSelect'svalueparameter, to matchSelectOptionPropsAdditional issues:
This PR wold allow for the removal of a bunch of
ts-expect-errors I added in openshift/console#14783