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

'event.target.value' are 'undefined' of 'onChange(event)' for 'Checkbox', 'Radio', and 'Dropdown' #638

Closed
twang2218 opened this issue Oct 6, 2016 · 10 comments
Labels

Comments

@twang2218
Copy link

Details

Version: "semantic-ui-react": "^0.54.2"

To reproduce the issue, here is the onChange() function:

function onChange(event) {
    console.log(`event.target.value: ${JSON.stringify(event.target.value)}`);
}

and use the following JSX for the testing:

const App = () => (
  <Container>
    <Divider hidden />
    <Header as='h1'>Checkbox</Header>
    <Segment>
      <Checkbox toggle label="Toggle" onChange={onChange}/>
      <Divider hidden />
      <Radio label="Radio" onChange={onChange} />
      <Divider hidden />
      <Input type="text" onChange={onChange} />
      <Divider hidden />
      <Dropdown selection options={options} placeholder='Dropdown' onChange={onChange} />
    </Segment>
  </Container>
)

Here is the sample code link for the test.

http://codepen.io/twang/pen/dpdowZ

Just open the Developer Tools, and check the Console, and then change each components' value.

For the <Input>, the event.target.value returns correct value of the <input ...>, however, for the <Checkbox>, <Radio> and <Dropdown>, the event.target.value are undefined.

@levithomason
Copy link
Member

levithomason commented Oct 6, 2016

This is a known limitation. The event.target is a browser event property. However, many Semantic UI components, such as a Dropdown, Checkbox, and Radio do not work directly with native browser form controls such as input and select. They are built using stylized markup and custom internal state.

Because of this, there are no native browser events available for certain callbacks. This is why all change events in Semantic-UI-React provide the event first (when available) but also a second argument which contains the data you need. You should never have to access the native browser event for most tasks.

You can see examples of how to retrieve values from the second argument in the docs. Such as the Radio Group example.

Going to close this issue as this is not a bug. Feel free to open another issue if you feel any components are missing helpful data in the second argument and we can add more callback data.

@jeffcarbs
Copy link
Member

jeffcarbs commented Oct 6, 2016

@twang2218 - to add to Levi's response, in the <Checkbox>, <Radio> and <Dropdown> components the second argument to onChange will be an object with name, value, and checked (if relevant). We're also considering just passing the whole props object back in addition to those keys (see #623).

@levithomason - for consistency, what do you think about returning { name, value } as the second argument to the onChange handler? Right now inside a Form component in my app I have:

handleDropdownChange (e, result) {
  const { name, value } = result
  // ...
}

handleInputChange (e) {
  const { name, value } = e.target
  // ...
}

which isn't ideal. I guess this would actually be addressed if we decided to pass all props back to handlers.

@levithomason
Copy link
Member

To keep things as simple and predictable as possible I think I'd like to push usage toward:

handleDropdownChange (e, data) {
  const { name, value } = data
  // ...
}

handleInputChange (e, data) {
  const { name, value } = data
  // ...
}

Where data may in fact be props per the issue you linked. We pass the event as a nicety really, just in case the user needs to handle an edge case. However it is inconsistent as noted.

@levithomason
Copy link
Member

levithomason commented Oct 6, 2016

I'm almost wondering then if it makes sense to pass the data first. This way the second parameter is the sometimes available event.

handleDropdownChange (props, e) {
  const { name, value } = props
  // ...
}

handleInputChange (props, e) {
  const { name, value } = props
  // ...
}

Thoughts?

@twang2218
Copy link
Author

The problem came from using semantic-ui-react with redux-form. The <Field> in redux-form will provides an onChange() callback in props, which expect the first argument to be React SyntheticEvent, or the value of the field.

Currently, I just wrap the <Checkbox > or <Dropdown> in another component, which provides a handleChange(event, data) function, which will call this.props.onChange(data.value) or this.props.onChange(data.checked). So, it would be nice to have less boilerplate code to use them. But, it's ok. I understand if it's not possible especially for something like <Dropdown>.

As you suggested, I tried the second argument of onChange(), it works, thank you. But the interfaces between those Form components are not consistent. For example, I should use data.value for <Dropdown>, however, the data.value for <Checkbox> are not defined, I should use data.checked instead. Maybe it would be better to have a consistent interface.

Another problem is that, although <Checkbox> is a boolean value component, the value props of <Checkbox> is expected to be a string, which caused some problem when passing <Field>'s input props down to <Checkbox>, I have to do some workarounds here as well.

@levithomason
Copy link
Member

levithomason commented Oct 7, 2016

For example, I should use data.value for , however, the data.value for are not defined, I should use data.checked instead. Maybe it would be better to have a consistent interface.

To be clear, this is not our design. It is how React handles Forms: https://facebook.github.io/react/docs/forms.html. It is also worth noting that this is how HTML handles checkboxes vs other inputs as well:

<input type='checkbox' value='terms' checked />

I think it is best to stick with the existing HTML and React conventions.

Another problem is that, although is a boolean value component, the value props of is expected to be a string, which caused some problem when passing 's input props down to , I have to do some workarounds here as well.

The checked attribute is boolean which indicates whether or not it is checked. The value is what you'd want to use in a Form to indicate what is checked. Consider the value='terms' example above, if also checked, then you know the user has agreed to the terms. See the Form onSubmit example and note the serialized form data object. All form controls are supported correctly.

@qoalu
Copy link
Contributor

qoalu commented Dec 13, 2016

@levithomason what is the status on 2 parameters for all input types for consistency?

Right now Checkbox action returns a javascript object and Input action returns a DOM Node with properties.

@levithomason
Copy link
Member

Most were done and released in #951 (v0.62). There is a checklist in #623, but it needs to be updated to reflect the work done in #951.

If you believe you are seeing a bug, please open a new issue and include the items requested in the issue template.

@AdventureBear
Copy link

AdventureBear commented May 29, 2018

I've tried implementing these suggestions and I still cannot get "data" returned from a Form element in a react application. Additionally, the linked documentation above that @levithomason posted above gets redirected to the Introduction section for semantic UI.

Can you link to current documentation for a solution?

@layershifter
Copy link
Member

@Semantic-Org Semantic-Org locked as resolved and limited conversation to collaborators Jun 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants