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

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

Open
bryceosterhaus opened this issue May 11, 2023 · 6 comments
Labels
type: bug Issues reporting that Component is not doing what should be done

Comments

@bryceosterhaus
Copy link
Member

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

@bryceosterhaus bryceosterhaus added the type: bug Issues reporting that Component is not doing what should be done label May 11, 2023
@ethib137
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Issues reporting that Component is not doing what should be done
Projects
None yet
Development

No branches or pull requests

3 participants