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

Checkbox: double call of onClick when event fired by mouse #3348

Closed
Obiwarn opened this issue Dec 20, 2018 · 11 comments · Fixed by #3351
Closed

Checkbox: double call of onClick when event fired by mouse #3348

Obiwarn opened this issue Dec 20, 2018 · 11 comments · Fixed by #3351
Labels

Comments

@Obiwarn
Copy link

Obiwarn commented Dec 20, 2018

e.g. use the example from the docs and change the Checkbox onChange to onClick:

import React, { Component } from 'react'
import { Button, Checkbox } from 'semantic-ui-react'

export default class CheckboxExampleRemoteControl extends Component {
  state = { checked: false }
  toggle = () => this.setState({ checked: !this.state.checked })

  render() {
    return (
      <div>
        <Button onClick={this.toggle}>Toggle it</Button>
        <Checkbox label='Check this box' onClick={this.toggle} checked={this.state.checked} />
      </div>
    )
  }
}
@layershifter
Copy link
Member

We introduced some changes in #2748, and yes it comes from there. I used your code in sandbox, so we have reproduction:
https://codesandbox.io/s/l9n88k3y4q

The issue comes from a double called onClick handler when it's clicked with mouse. Should be fixed.

@layershifter layershifter changed the title BUG: Not sure, but is onClick broken on checkbox in 0.84? Checkbox: double call of onClick when event fired by mouse Dec 23, 2018
@Fabianopb
Copy link
Member

@layershifter I'd be happy to contribute to this one!

@layershifter
Copy link
Member

@Fabianopb you're welcome 👍

@Fabianopb
Copy link
Member

I've been investigating this story, which is related to #2587 and I'm wondering about the order that the events are called. Currently we have

handleMouseDown() --> handleMouseUp() --> handleChange() --> handleClick()

Shouldn't handleClick() be called before handleChange()? Ping @levithomason

@Fabianopb
Copy link
Member

Fabianopb commented Dec 24, 2018

PR created! #3351

@levithomason
Copy link
Member

Heads up, I've got some local work on this that I'm implementing. I'll push later tonight or tomorrow and give an update on where it is.

@Fabianopb
Copy link
Member

@levithomason would this impact any work carried out in #3351? The PR is ready to merge IMO, what do you think @layershifter?

layershifter pushed a commit that referenced this issue Jan 16, 2019
* fix(Checkbox): Let click handler call onClick to avoid duplicate calls (#3348)

* fix(Checkbox): Fix test to call onClick from a click event (#3348)

* fix(Checkbox): Move onClick call completely to handleChange (#3348).

* fix(Checkbox): Create tests for DOM Comparisons (#3348).

* revert change

* Update Checkbox-test.js

* Update Checkbox-test.js

* Update isConformant.js

* fix(Checkbox): Fix typo in handleClick comment (#3348).

* fix(Checkbox): Add tests for controlled component with setState as function (#3348).

* fix(Checkbox): ensure onClick is called

* fix(Checkbox): Fire DOM event on controlled component tests to emulate the real behavior (#3348).

* fix(Checkbox): Completely remove click handler (#3348).

* small cleanup
@NullVoxPopuli
Copy link

has this been released? I'm on 0.84 and am witnessing double onClicks

@layershifter
Copy link
Member

Will be released in 0.85.0.

@NullVoxPopuli
Copy link

when will that be released?

@levithomason
Copy link
Member

Releasing today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants