[PHC-4158] SelectOption Title Typing

[MMFane]

What Was Changed

• To fix a type error for this PR, adding the typed ReactNode and ReactNode[] to <SelectOption /> titles
• The error came about by using react-intl's imperative API to add a styled <i> to the title, which makes the title's type a ReactNode[] not a string.

Questions for the Team

• There are currently no stories for SelectOption in the storybook - do you think I should create some?
• This could be potentially useful in other components where people might want to use these styled sub elements
  • i.e. the <MenuItem /> is essentially the same use case, etc.
  • Should we make an effort to update all potentially applicable use cases now, or go on a need-to-build basis?

Screenshots

see linked PR for a visual example

[evanheisler]

There are currently no stories for SelectOption in the storybook - do you think I should create some?

I don't know if more stories provide much value, if the options are essentially just styled text w/o any behavior of their own.

This could be potentially useful in other components where people might want to use these styled sub elements

I'd say be fairly prescriptive where to add looser types, if for no other reason than to avoid opening up a bunch of new edge cases and test scenarios. Technically, allowing an array of ReactNode's as an option title could create problems, even if the use case here is an i18n object.

[dexterca]

There are currently no stories for SelectOption in the storybook - do you think I should create some?

Can we include an example in Select.stories.tsx? I think it would be handy for me to reference some kind of example in storybook.

Should we make an effort to update all potentially applicable use cases now, or go on a need-to-build basis?

I'm good with going on a need-to-build basis

[MMFane]

I'd say be fairly prescriptive where to add looser types, if for no other reason than to avoid opening up a bunch of new edge cases and test scenarios. Technically, allowing an array of ReactNode's as an option title could create problems, even if the use case here is an i18n object.

I think it makes sense to be cautious. I'm curious, what's on your mind in terms of problems and risks introduced by adding ReactNode[]? Trying to gauge if this change is worth it.

If we decide to move forward with this I can add a story to Select for an example, @dexterca.

[evanheisler]

I'd say be fairly prescriptive where to add looser types, if for no other reason than to avoid opening up a bunch of new edge cases and test scenarios. Technically, allowing an array of ReactNode's as an option title could create problems, even if the use case here is an i18n object.

I think it makes sense to be cautious. I'm curious, what's on your mind in terms of problems and risks introduced by adding ReactNode[]? Trying to gauge if this change is worth it.

My mind went to someone trying to add some sort of complex interactive component as a "title", which I guess isn't really a problem for the library, we just wouldn't be warning someone of a potential mistake when in most cases this prop is just a simple string.

I think you have a legitimate use case, but for arguments' sake I wonder if you should just cast it locally to override the type as needed, or if we want this type to be unknown.

[MMFane]

@evanheisler Yeah, casting would be a great solution for now. Since this is the only use-case so far, I agree that modifying Chrōma is a little extreme. And I think making the type unknown could potentially be more confusing or open to interpretation. Thanks for thinking through this with me.