fix(Checkbox): onFocus and onBlur events

[tarang9211]

Fixes #1335

[levithomason]

Rather than adding callbacks, we need to set focus and add tests, see #1335 (comment).

[codecov-io]

Codecov Report

Merging #1361 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1361      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         141      141              
  Lines        2371     2376       +5     
==========================================
+ Hits         2365     2370       +5     
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Checkbox/Checkbox.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fd65da...1b69037. Read the comment docs.

[tarang9211]

@layershifter does this look right? what about the blur event?

[levithomason]

I left a comment on the issue for the blur event. Let's get tests passing as well.

[tarang9211]

Sweet thanks! For the test, we're simulating a onMouseDown event right?

[levithomason]

Indeed.

[levithomason]

Checkbox
  ✖ handles events transparently
    PhantomJS 2.1.1 (Linux 0.0.0)
    <Checkbox onMouseDown={handleMouseDown} />
                           ^ was not called once on "mouseDown".
                             You may need to hoist your event handlers up to the root element.

Since the onMouseDown is now captured, we need to also call this.props.onMouseDown in our handler with the original event. This way, the user's prop is also called. Otherwise, we are intercepting it and swallowing it.

Since we're now handling the prop, it also needs added to propTypes. In order to keep consistent with our callback signatures, we should pass this.props as the second callback parameter. You can reference many other component's callbacks for examples on how to add the propType doc block and how to pass back the right arguments.

[levithomason]

Need a line break between the props here.

[levithomason]

Still need the line break here 😉

-        onClick={this.handleClick} onChange={this.handleClick}
+        onClick={this.handleClick}
+        onChange={this.handleClick}

[tarang9211]

you mean something like this @levithomason?

[levithomason]

I've clarified the changes necessary here.

[levithomason]

This is correct, however, we also need to call the user's onMouseDown prop here if they provide one. That is because we handling the prop, so it isn't auto spread on the rendered component. Even if it were, we are also setting the onMouseDown event ourselves, so our handler is the only one that will ever be called.

So, if they pass:

<Checkbox onMouseDown={() => console.log('hi')} />

Then nothing will happen onMouseDown. To ensure they can also pass a onMouseDown handler, we need to also call it in our handler for them. Since mouse down is called before the focus event, let's call it in the right order as well:

handleMouseDown = (e) => {
  _.invoke('onMouseDown', this.props, [e])
  _.invoke('focus', this.checkboxRef)
}

[levithomason]

Still need the line break here 😉

-        onClick={this.handleClick} onChange={this.handleClick}
+        onClick={this.handleClick}
+        onChange={this.handleClick}

[tarang9211]

Sounds good @levithomason. I'll get to this in a bit.

[tarang9211]

@levithomason Just pushed a fix that takes care of this issue, I believe. Let me know if we need anything else.

[levithomason]

Changes look good, we just need some tests for this and we can merge 👍

[tarang9211]

@levithomason I should also add onMouseDown to the component props, right? Similar to how onClick and onChange are?

[tarang9211]

@levithomason without adding any test cases, this one seems to fail. are we not exposing the prop properly at the user level?

[tarang9211]

@layershifter tests still fail locally.

[layershifter]

Yep, I see. Will back there tomorrow 😴

[tarang9211]

@layershifter pushed a new commit, it could be a potential fix. Let me know if it looks ok

[levithomason]

This component looks good conformance as far as conformance tests go. We now just need a test that asserts that:

it('sets focuses the input onMouseDown', () => { ... })

[tarang9211]

it('sets focuses the input onMouseDown', () => { ... })

What exactly should be checked in this test? I was basing my implementation on the onClick and onChange tests. Is that a good approach?

[levithomason]

The mouseDown event should be fired on the outer checkbox div and an assertion should be made that the document.activeElement is indeed the input element.

[layershifter]

@levithomason I've made some cleanups and added test

[levithomason]

💥 thanks much!

[levithomason]

Released in [email protected].