Skip to content

Conversation

@logonoff
Copy link
Member

@logonoff logonoff commented Jul 23, 2025

What: Closes #11361

Non-breaking change to loosen the expected type of onSelect's value parameter, to match SelectOptionProps

Additional issues:

This PR wold allow for the removal of a bunch of ts-expect-errors I added in openshift/console#14783

@patternfly-build
Copy link
Contributor

patternfly-build commented Jul 23, 2025

@nicolethoen nicolethoen requested a review from kmcfaul July 25, 2025 13:12
Copy link
Contributor

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

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?

@logonoff
Copy link
Member Author

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

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Menu's onSelect should also be updated to reference MenuItem's itemId.

@logonoff logonoff requested a review from kmcfaul July 30, 2025 17:24
@logonoff logonoff force-pushed the selectoption branch 2 times, most recently from c05fc3e to 599d525 Compare July 30, 2025 17:32
@kmcfaul kmcfaul merged commit d50ba6d into patternfly:main Sep 24, 2025
13 checks passed
@logonoff logonoff deleted the selectoption branch September 24, 2025 19:32
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-code-editor@6.3.1-prerelease.19
  • @patternfly/react-core@6.3.1-prerelease.19
  • @patternfly/react-docs@7.3.1-prerelease.23
  • @patternfly/react-drag-drop@6.3.1-prerelease.19
  • demo-app-ts@6.0.0-prerelease.157
  • @patternfly/react-table@6.3.1-prerelease.21
  • @patternfly/react-templates@6.3.1-prerelease.19

Thanks for your contribution! 🎉

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.

SelectOption.value and Select.selected has "any" type, but Select.onSelect has "(..., string | number)"

6 participants