liferay / clay

A web implementation of the Lexicon Experience Language
http://clayui.com
Other
208 stars 483 forks source link

ClayMultiSelect doesn't register an 'onChange' event when it is inside of a form #5524

Open bryceosterhaus opened 1 year ago

bryceosterhaus commented 1 year ago

https://codesandbox.io/s/quirky-snow-jrpflb?file=/src/index.js

What are the steps to reproduce?

Open up the console to see the logs. When typing into a ClayInput, the form's onChange fires when the input is typed into. But when you select an item in the MultiSelect component, it doesn't register in the form's onChange prop. Note that the hidden input of MultiSelect does have the value change.

What is the expected result?

I would expect that the form's onChange would pick up new values when they added to the MultiSelect's hidden input values.


I also noticed that the multiSelect creates a new hidden input for every new item selected, I think this might be wrong and it should instead aggregate all the values into something like <input value="one,two" type="hidden" />.

Screenshot 2023-05-11 at 2 03 39 PM

ethib137 commented 1 year ago

I also noticed that the multiSelect creates a new hidden input for every new item selected, I think this might be wrong and it should instead aggregate all the values into something like .

Hey @bryceosterhaus for this one, it is a little odd, but portal currently uses it that wya in many places. Most notably assetCategories and assetTags use it in this way. You'd need to make sure it wouldn't break the existing uses in Portal.

bryceosterhaus commented 1 year ago

@ethib137 is it expected they have the same nametoo?

For some reason I thought name must be unique, but according to this, it is expected that checkboxes can have the same name.

ethib137 commented 1 year ago

Yes, same name is allowed for all inputs, and it’s expected in this case. When you access the Param on the backend it returns an array of all the values.

bryceosterhaus commented 1 year ago

ah I see.

I think my original issue still stands though... I think changing the values in MultiSelect should also reflect in a forms onChange

matuzalemsteles commented 1 year ago

I'm looking into this right now, we may probably have some native bugs here that won't work correctly or maybe due to how the multi select behavior works, for example adding labels as commented above creates an input[hidden] for each label so that it arrives at the backend as an array, I think that adding a new input with the value of the label will not call the onChange of your form just in case you change the value of that input which is not the case.

Unfortunately I don't think we can change this behavior or try to simulate it for the MultiSelect labels case, I think what we can do here is just add the name to the MultiSelect input that is going to the one input[hidden] which is not necessary in my view here.

I would say that you could use Formik here to control all the states of your form it would be "simpler" to deal with dynamic components like MultiSelect.

bryceosterhaus commented 1 year ago

I think I tried formik with a Form and MultiSelect and it didn't work properly because it wasn't able to hook into the onChange of multi-select.

Do you have an example of formik + multiSelect working?