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(Search): only call onBlur & onFocus event handler once #1963

Merged
merged 4 commits into from
Aug 20, 2017

Conversation

chopstikk
Copy link
Contributor

@chopstikk chopstikk commented Aug 14, 2017

Fixes #1962

Search component onBlur and onFocus event handers should not be called twice.

@codecov-io
Copy link

codecov-io commented Aug 14, 2017

Codecov Report

Merging #1963 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1963   +/-   ##
======================================
  Coverage    99.8%   99.8%           
======================================
  Files         148     148           
  Lines        2568    2568           
======================================
  Hits         2563    2563           
  Misses          5       5
Impacted Files Coverage Δ
src/modules/Search/Search.js 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 a4780cd...c80777d. Read the comment docs.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

@chopstikk Thanks for PR. However, I think that we should add tests there to omit such cases in future.

Something like:

describe('onFocus', () => {
  it('is called with (e, data)', () => {
    const onFocus = sandbox.spy()
    wrapperMount(<Search onFocus={onFocus} />)
    
    wrapper.simulate('focus')

    onFocus.should.have.been.calledOnce()
    onFocus.should.have.been.calledWithMatch({ }, { options })
  })
})

@@ -525,6 +525,28 @@ describe('Search', () => {
})
})

describe('onBlur', () => {
it('is called with (event) on search input blur', () => {
const spy = sandbox.spy()
Copy link
Member

Choose a reason for hiding this comment

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

Please name spy as event handler, i.e. onBlur.

.simulate('blur', nativeEvent)

spy.should.have.been.calledOnce()
spy.should.have.been.calledWithMatch(nativeEvent)
Copy link
Member

Choose a reason for hiding this comment

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

Let's also check that second argument provides a valid props:

-wrapperMount(<Search onBlur={spy} />)
+wrapperMount(<Search onBlur={spy} results={options} />)

-spy.should.have.been.calledWithMatch(nativeEvent)
+spy.should.have.been.calledWithMatch(nativeEvent, { onBlur, results: options })

@@ -525,6 +525,28 @@ describe('Search', () => {
})
})

describe('onBlur', () => {
it('is called with (event) on search input blur', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's also correct a title is called with (e, data) on...

@levithomason levithomason merged commit a655123 into Semantic-Org:master Aug 20, 2017
@levithomason
Copy link
Member

I love you two, thanks! 😄

@levithomason
Copy link
Member

Released in semantic-ui-react@0.71.5

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

Successfully merging this pull request may close these issues.

None yet

4 participants