Clay: Missing ClayMultiSelect onItemsAdded and onItemsRemoved callbacks

[carloslancha]

Right now the component provides onItemsChanged API, but we need to know when items changed because an item was added or removed.

[matuzalemsteles]

hey @carloslancha can you describe your use case to receive this information? I think it would help build something better.

hey @bryceosterhaus I think to improve this information we can treat onItemsChanged like useReducer, just have the same signature, passing payload and type.

Something like that:

enum Type {
    Added = 1,
    Removed = 0,
}

interface IItemsChanged {
    payload: Array<string>;
    type: Type;
}

onItemsChanged={({payload, type}) => {...}};

[carloslancha]

@matuzalemsteles in the AssetTagsSelector tag component we are receiving strings addCallback and removeCallback, two callback names to call global methods on items added and removed.

    _notifyItemsChanged(callback, item) {
        if (callback) {
            window[callback](item);
        }
    }

    _handleItemRemoved(event) {
        this.selectedItems = event.data.selectedItems;
        this.tagNames = this._getTagNames();

        this._notifyItemsChanged(
            this.removeCallback,
            event.data.item
        );
    }

[bryceosterhaus]

You should be able to just compare the old state items versus the new items from the callback, for example:

<ClayInputWithMultiSelect
    items={items}
    onItemsChange={itemsChanged => {
        const removedItems = items.filter(value => !itemsChanged.includes(value));
        const newItems = itemsChanged.filter(value => !items.includes(value));
    }}
/>

@matuzalemsteles I think using a reducer type callback would be a bit much for this use case. I think it would cause more confusion than clarity.

[matuzalemsteles]

Treating him as a reducer is just having the same dispatch signature, don't have the full useReducer thought here 🙂 just not to cause other thoughts. I think this information is valuable and clearer, having to compare the values ​​is a bit of a shady way of doing things ... I think it improves the experience of using our API.

We don't need to use enum in type, it's just a way I like to refer to numbers instead of adding text... we can replace it with text for simplicity.

[bryceosterhaus]

having to compare the values ​​is a bit of a shady way of doing things

This is actually a fairly standard practice in the react community. Typically controlled components will be compared with previous state for any knowledge of added or removed.

I think it improves the experience of using our API.

If we go the route of adding a "reducer type" call is that we start to deviate from a standard of "controlled." My main concern is consistency between components, if we start to adopt different call signatures for onChange type callbacks, its going to make the API inconsistent. The other option would be to adopt the reducer type callback across all components, but IMO I think that would be overkill. Long story short, I think we need to keep all controlled components consistent in its callback signature.

[matuzalemsteles]

This is actually a fairly standard practice in the react community. Typically controlled components will be compared with previous state for any knowledge of added or removed.

When I mean this is a bit obscure, it means that people will have to think about how to do that, we have different levels of people at Liferay, expecting them to adopt this pattern can be a failure. This is a door for people to start creating bad practices to solve the same problem.

If we go the route of adding a "reducer type" call is that we start to deviate from a standard of "controlled." My main concern is consistency between components, if we start to adopt different call signatures for onChange type callbacks, its going to make the API inconsistent. The other option would be to adopt the reducer type callback across all components, but IMO I think that would be overkill. Long story short, I think we need to keep all controlled components consistent in its callback signature.

Yes that is a problem.

Can we adopt some kind of hook that can help people solve this in some general way?

[jbalsas]

Hey @matuzalemsteles, I totally get where you're coming from and you're right to voice your concerns. At the same time, I fear we might be entering in an old and also dangerous pattern of trying to fix everything for everyone, which can also lead to failure.

I think we don't have so many usages yet to prevent us from being a bit more conservative before committing to a more complete feature set.

We have time iterate and improve, so I'd recommend to stay on the lean side for now and work with implementors to understand our own framework from the outside. Then come back and put that gained knowledge into improving our lovely library!

[matuzalemsteles]

Hey @matuzalemsteles, I totally get where you're coming from and you're right to voice your concerns. At the same time, I fear we might be entering in an old and also dangerous pattern of trying to fix everything for everyone, which can also lead to failure.

Yes you are right that we do not need to fix everything for everyone... but the ideal would be to show the way. There are things they should seek knowledge of course...

I think we don't have so many usages yet to prevent us from being a bit more conservative before committing to a more complete feature set.

Yeah, it's good to be conservative but it's a little confusing because as we are following a specification and not implementing it "fully" or showing the paths, it may sound like inconsistency (which I keep listening to...). So I get a little worried and confused about what message we should get across at the beginning. Since we want to be conservative... I say that because people want something fast and they have a wrong view of Clay...

[jbalsas]

but the ideal would be to show the way. There are things they should seek knowledge of course...

Let's start by showing the way rather than building it, then! 😉

So I get a little worried and confused about what message we should get across at the beginning. Since we want to be conservative... I say that because people want something fast and they have a wrong view of Clay...

We're at the beginning. We can't do it all at once. We'll get there, and we need to do it in a sustainable way. When in doubt, it's probably better to defer a decision until we have more field experience and information (as long as what's being asked is already possible but just inconvenient). A premature abstraction can usually be more harmful than code duplication.

For example, in this case, there's already a precedent in search-tuning-synonyms-web where they already implement their own filter logic.

I'm not saying we should not or won't implement this, just that we can take our time to make long-lasting decisions. In the meantime, as you said, let's add more docs, examples and samples so our devs know how to do things!

[bryceosterhaus]

added example at https://github.com/liferay/clay/pull/2627