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

Enhanced Select to handle "selecting objects" #3893

Open
jskupsik opened this issue Jan 8, 2025 · 0 comments
Open

Enhanced Select to handle "selecting objects" #3893

jskupsik opened this issue Jan 8, 2025 · 0 comments
Assignees
Labels

Comments

@jskupsik
Copy link
Contributor

jskupsik commented Jan 8, 2025

A not-too-uncommon use case of the select prop is to let the user select (conceptually) an object.

Our select component almost supports this, but not really well and not all the time.

There are two big concerns here:

  1. We don't make it easy to always render a "value" using a "label" - it is just very finicky to get it right, and there are some edge cases/limitations that results in bugs where we render the value.
  2. We have a weak assumption that "value" is a primitive (string or number). This leads to complexity / errors in cases where we essentially want the object to be the value, but have to work around this select limitation.

Usually, we implement "object selection" by making our SelectOption have a value of the object's id and a label be the object's name. This works nicely most of the time, rendering the name rather than the id, but there are a lot of edge cases/pitfalls where we render the id instead:

  • The currently selected object is not currently selectable - dynamic options have to be carefully curated to never exclude the current object.
  • We are using QueryFn and we are rendering a saved value not in cache - we have to manually use options to prime the initial value, or give up on using the id and instead use the name as the key.
  • The form that wraps the select is readonly - never make a form that wraps a select readonly, instead make it disabled. For selectEditors, we need to remember give the column the proper renderer.

This makes using the select correctly for this purpose have some gotchas.

If we do not have all of the objects readily available, it is quite difficult to use queryFn with SelectObject. One is tempted to make the value be the object itself rather than id, it almost works, but there are issues with how it is handled and occasionally is rendered as [Object object]. We should consider supporting this.

We have a SelectOption - it is typed and assumed to be (optionally):

export interface SelectOption {
    value?: any;
    label?: string;
    options?: (SelectOption | any)[];
}

but in some places we instruct the user to place other fields on it - fields which then aren't always present if rendering a saved value not defined in options:

    /**
     * Function to render options in the dropdown list. Called for each option object (which
     * will contain at minimum a value and label field, as well as any other fields present in
     * the source objects).
     */
    optionRenderer?: (opt: SelectOption) => ReactNode;

In a lot of client apps - due to the limitation on value above - we add other fields to SelectOptions, such as "src" containing a reference to the source object. If we have every object and provide a whole options, we will have the "src" for every object. However, when using queryFn or a filtered options, we may run into the issues above, and the selected object may be rendered by its id or by [Object object] rather than by the label or renderer we want. I would like to make SelectOption generic - allow the user to add fields to it - and a way to specify a cache lookup method, so the select can request (either from API or locally) any object lookups it needs to render but that aren't in its cache.

As more of an aside, we also have the labelField and valueField select props, so a SelectOption may have none of the standard fields! There seems to be two design principles here - should we do:

select({
    options: myObjects,
    labelField: 'name',
    valueField: 'id'
})

or

select({
    options: myObjects.map(it => {label: it.name, value: it.id, src: it})
})

I tend to favor the second one, but I wonder if the typing here can be improved.

@jskupsik jskupsik added the forms label Jan 8, 2025
@jskupsik jskupsik self-assigned this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant