feat(combobox)!: prefer strings as option values #637
Conversation
| @@ -31,7 +31,7 @@ ComboboxContainer.propTypes = { | |||
| hasMessage: PropTypes.bool, | |||
| options: PropTypes.arrayOf(PropTypes.any).isRequired, | |||
| inputValue: PropTypes.string, | |||
| selectionValue: PropTypes.oneOfType([PropTypes.string, PropTypes.arrayOf(PropTypes.string)]), | |||
| selectionValue: PropTypes.any, | |||
There was a problem hiding this comment.
This didn't seem to hold up after the type change, and I didn't find a way to define an accurate statement here using prop-types (using mixes of PropTypes.oneOf and PropTypes.oneOfType). Open to ideas if this seems like an issue.
| @@ -8,7 +8,7 @@ | |||
| import { IUseFieldReturnValue } from '@zendeskgarden/container-field'; | |||
| import { HTMLProps, ReactNode, RefObject } from 'react'; | |||
|
|
|||
| export type OptionValue = string | object; | |||
| export type OptionValue = string; | |||
There was a problem hiding this comment.
I liked the idea of retaining this type definition for internal use in spotting where values are referenced in code.
There was a problem hiding this comment.
The problem is that this doesn't play nice with the generated docs. I would favor removing entirely so that the updated docs are very clear that the value is now string only.
Oops, I was looking at the wrong docs. Currently looks good, so I'm fine either way.
| @@ -8,7 +8,7 @@ | |||
| import { IUseFieldReturnValue } from '@zendeskgarden/container-field'; | |||
| import { HTMLProps, ReactNode, RefObject } from 'react'; | |||
|
|
|||
| export type OptionValue = string | object; | |||
| export type OptionValue = string; | |||
There was a problem hiding this comment.
The problem is that this doesn't play nice with the generated docs. I would favor removing entirely so that the updated docs are very clear that the value is now string only.
Oops, I was looking at the wrong docs. Currently looks good, so I'm fine either way.
geotrev
changed the title
Description
container-comboboxhas allowed consumers to setoption.valuewith anobjectfor ergonomics. This has proven to create unintended mismatches to selected values, and so this PR removes that compatibility feature, instead preferring allvalues to be strings for the best comparison.Checklist
npm start)💂♂️ includes new unit tests