Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TylerR909
Copy link
Contributor

In BluePrint I find that this is very tedious...

<SelectField
  options={options}
  getOptionValue={(o) => o.id}
  getOptionLabel={(o) => o.name}
/>

...given that it's id/value or displayName/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.

@TylerR909 TylerR909 changed the title Chore/dropdown label value defaults fix: dropdown label value defaults Jan 21, 2023
/** Defaults to `id ?? value` */
getOptionValue?: (opt: O) => V;
/** Defaults to `displayName || label || name || value` */
getOptionLabel?: (opt: O) => string;
Copy link
Contributor

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

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

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");
})();
Copy link
Contributor

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?

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.

None yet

2 participants