Clay: Multiselect is not controlling duplicated items

[carloslancha]

Actual result:
You can add the same item multiple times

Expected result:
You can't add the same item multiple times or, at least, make it configurable.

[bryceosterhaus]

From my understanding I think this sort of logic would be done in the parent component. Since MultiSelect is a controlled component, you'd just have to check the current items vs the new item in the onItemsChange callback.

[matuzalemsteles]

hey @bryceosterhaus I've been thinking about this too, but I think we can cut back on the work you have to do every time you use this component, it will be common for everyone who uses MultiSelect. Then we can add something on our side here.

[carloslancha]

hey @bryceosterhaus as far as I know this is the default behavior of the component, so if we let this out we'll be "forcing" the developers to implement the same logic to avoid duplicated items everywhere

[bryceosterhaus]

Is this documented anywhere from lexicon? I was parsing through some of the docs and didn't see anything in regards to a specific specification for this.

My concern for adding this into the internals of this component is that it is now tightly coupled with onItemsChange prop, meaning a user can achieve the same functionality but go about it in two different ways. The more we can "lock in" to the controlled aspect of form components, we can keep consistency across controlled components. If we add specific internal logic to disable duplication, we now start to add internal complexity that interacts with the controlled aspect of the component.

I would suggest that instead of adding an additional prop, we create a high-level component that wraps the default multi-select with a specific onItemsChange. It could be something like

const MultiSelectWithDedupe = ({items, onItemsChange, ...others}) => {
    return (
        <ClayInputWithMultiSelect
            {...others}
            items={items}
            onItemsChange={newItems => {
                onItemsChange(newItems.filter(value => !items.includes(value)));
            }}
        />
    );
};

By doing this, we keep multi-select very pure and controlled, but also give ease of access to this specific logic.

[carloslancha]

@drakonux could you please take a look on this?

[drakonux]

Hi guys!

I was reviewing the documentation and it says:

The autocomplete rules: This set of rules must be elaborated by you as they must fit your use case. You need to create your acceptance criteria to consider a string valid to create a label for the input field.

So, I think Lexicon Team decided to leave the rules on the side of the product instead of the component. However, I think @carloslancha is right. I can't imagine a scenario where the component needs to behave selecting the same item more than one. So from my side, we are aligned with that change.

For now, I will include a reference to control duplicates in the autocomplete rules section.

[matuzalemsteles]

Disallow repeated labels is described in session "Create an already existing label"(https://docs.google.com/document/d/1cIx6RkWNbnho_fOSR__3Fp5alIBE_ObhO-jecfXlA9I/edit#).

[drakonux]

So it was already added. So sorry, I didn't see it. Thanks @matuzalemsteles

ps. removing my last update in the definition doc 🤦‍♂️

[matuzalemsteles]

I would suggest that instead of adding an additional prop, we create a high-level component that wraps the default multi-select with a specific onItemsChange. It could be something like

@bryceosterhaus I'm not so inclined to create another high-level component on top of MultiSelect to deal with just this logic, i'm just worried about creating a lot of components like this.

I agree that we should make the controlled components pure and little logic attached to them, that's their tradeoff, I understand that. When I say that, I mean our components that we deliver, on the front line by design we deliver as if they were an uncontrolled component and people have to control so that everything works well.

Once we decide to go that way we should start designing our documents and storybook to show people how to get to the specs.

We should try to cover the rules of the components on our side and we can decide how we will approach this, whether it's a hook, high-level or low-level. We can expose it as we see fit, but we should help developers reach the desired end result...

[jbalsas]

For now, most of the work is being done by us, so I think we can postpone some of these decisions until we have a clearer picture of what's absolutely necessary, what's desirable and what can be left to our fellow developers.

In cases like this, since we (@carloslancha and @julien ) are driving the implementation, I think we should be fine adding this on the side rather than cramming it into Clay off the bat. Once we have enough experience we should be able to circle back and bring more ergonomics over here.

We need to make it simple for developers to build apps that follow the specs... but we also don't need to do it all at once. I fear we risk death by a thousand cuts and analysis paralysis if we try. I'd say we could keep it small and open, move on to help teams implement and discover weaknesses and then iterate on top.

[bryceosterhaus]

Going to close. Will re-open if this comes up again.