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

Fix SelectInput / SelectArrayInput onChange handler #7519

Merged
merged 4 commits into from
Apr 27, 2022
Merged

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented Apr 11, 2022

Fixes #7518

SelectInput / SelectArrayInput change event handler should receive the event object unless the change was fired due to the creation of a new item, in that case it should receive the new value

@WiXSL WiXSL added this to the 4.0.0-rc.2 milestone Apr 11, 2022
@WiXSL WiXSL added the RFR Ready For Review label Apr 11, 2022
Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve the changes, but I have a few questions:

  • Are SelectArrayInput and SelectInput the only components affected by this? What about TextInput and the others for example?
  • Why do we need to do a difference with what we send to the onChange callback when we add an item vs when we select another? Is it documented somewhere?

@fzaninotto
Copy link
Member

Also, it seems the accepted onChange arguments are fuzzy: event or value? We should choose one, and expect it everywhere. I don't mind if we support both for BC, but the types (and documentation) must be clear about the idiomatic way.

@WiXSL
Copy link
Contributor Author

WiXSL commented Apr 12, 2022

I approve the changes, but I have a few questions:

  • Are SelectArrayInput and SelectInput the only components affected by this? What about TextInput and the others for example?

The only other inputs that do not pass the event object are AutocompleteInput and AutocompleteArrayInput (that I know of)
According to @djhi, this comes from problems with final-form, maybe It could be improved now that we use react-hook-form

  • Why do we need to do a difference with what we send to the onChange callback when we add an item vs when we select another? Is it documented somewhere?

As of this PR, my intention was just to maintain the functionality of master preventing a BC.
But I think the answer is in the fact that you can create new items, and in that case you don't have the value in the event object.
Maybe this can be changed, to pass two arguments, the event object and the new item to the onChange event handler.

I don't think all of this behavior is properly documented.

@WiXSL
Copy link
Contributor Author

WiXSL commented Apr 12, 2022

Also, it seems the accepted onChange arguments are fuzzy: event or value? We should choose one, and expect it everywhere. I don't mind if we support both for BC, but the types (and documentation) must be clear about the idiomatic way.

Ok, so, to be clear, do you want me to use this PR to change the onChange event handlers of these inputs to always pass the event object?
In the case of newly created items, a second arguments must be used then, since the event object doesn't include this new value

@fzaninotto fzaninotto removed this from the 4.0.0 milestone Apr 13, 2022
@WiXSL WiXSL added this to the 4.0.1 milestone Apr 13, 2022
@vercel vercel bot temporarily deployed to Preview – react-admin April 13, 2022 12:32 Inactive
@djhi djhi merged commit 1e773c1 into master Apr 27, 2022
@djhi djhi deleted the fix-select-onchange branch April 27, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v4.0.0-rc.1] SelectInput onChange type incorrect
4 participants