-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: dropdown label value defaults #792
base: main
Are you sure you want to change the base?
Conversation
/** Defaults to `id ?? value` */ | ||
getOptionValue?: (opt: O) => V; | ||
/** Defaults to `displayName || label || name || value` */ | ||
getOptionLabel?: (opt: O) => string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: Some of this should already be handled by the SelectField
overload:
export function SelectField<O, V extends Value>(props: SelectFieldProps<O, V>): JSX.Element;
export function SelectField<O extends HasIdAndName<V>, V extends Value>(
props: Optional<SelectFieldProps<O, V>, "getOptionValue" | "getOptionLabel">,
): JSX.Element;
export function SelectField<O, V extends Value>(
props: Optional<SelectFieldProps<O, V>, "getOptionLabel" | "getOptionValue">,
I.e. it conditionally Optionalize
s getOptionValue
and getOptionLabel
only when O
has an id and name.
Granted, that was very inflexible, but it is more type-safe; can you try working the AtLeastOne
trick as a replacement for O extends HasIdAndName
, so that ideally we get both the increased flexibility of "use any one of these N fields" while not having to fallback on runtime checks for callers not passing the properties when they should?
(I had tried making this more flexible in the past, but didn't have the AtLeastOne
trick, so it led to ~an egregious amount of overloads, like 1 overload per guessed label, 1 overload per guessed value, and so just gave up.)
|
||
type InferrableOptionValue = AtLeastOne<{ | ||
id: any; | ||
value: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: granted, this is not a public API, but would prefer using Value
instead of any
.
displayName: any; | ||
name: any; | ||
label: any; | ||
value: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: Ideally these would be string
... or string | ReactNode
?
o?.value || | ||
(() => { | ||
throw new Error("Unable to determine option label. Please provide a custom getOptionLabel function"); | ||
})(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Surprised we don't have fail
copy/pasted in, but seems worth it?
In BluePrint I find that this is very tedious...
...given that it's
id/value
ordisplayName/label/name/value
seemingly 95% of the time. So, let's make getOptionValue/Label optional and try to infer, while still easily letting developers override or provide missing behavior when necessary.Also, I tried to type-guard this (like if you pass in
{ code: string; id: string }[]
it'd tell you getOptionLabel is required), but our abundant use of Interfaces over Types makes that a significant lift. Interfaces may only extend other Interfaces with statically-known types, so a key that's conditionally required or optional breaks extensions and a few overloads.