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

Enhance internal state control by providing onOuterClick prop #208

Merged
merged 1 commit into from Oct 4, 2017
Merged

Enhance internal state control by providing onOuterClick prop #208

merged 1 commit into from Oct 4, 2017

Conversation

@divyenduz
Copy link
Collaborator

@divyenduz divyenduz commented Oct 2, 2017

This PR is not ready to be merged yet. When merged, this will fix #206

What:

Add an optional prop onOuterClick to better handle the internal state or anything else with downshift

Why:

This PR is required to enhance the developer experience around outer click and internal state handling with downshift with issues like #206

How:

This PR does the following (Will appreciate early feedback and comments):-

  1. Add onOuterClick description to README. I can remove the link to a specific issue from there

  2. Add onOuterClick to propTypes with type PropTypes.func

  3. Call this.props.onOuterClick as a callback to reset function when it is called under onMouseUp event

  4. Add a test case for newly added functionality. This is still incomplete. I started with the test case expecting it to fail in its current state where I open the menu and simulate an outside click with a click on document.body. With the current state of the onOuterClick callback, this should have resulted in isOpen to be false there by failing the test but it is not the case. Exploring it further but this makes this PR not "ready to be merged".

  5. Also, to simulate the click on document.body. I borrowed the function mouseDownAndUp from downshift.lifecycle.js. I can either move this test over there or expose that function in a better way than duplicating.

  6. I observed that package.json got formatted as soon as I run npm install. I have not checked that in with this PR. Not sure why that happened, I have validated that it happens with npm install and not via other action. Any pointers on that?

Let me know your feedback on the approach? Also, please point out if you believe that I understood the problem incorrectly.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table
@tansongyang
Copy link
Collaborator

@tansongyang tansongyang commented Oct 2, 2017

If you're comfortable with TypeScript, I would also suggest updating the typings. If not, don't worry; someone else can do it.

@codecov-io
Copy link

@codecov-io codecov-io commented Oct 2, 2017

Codecov Report

Merging #208 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #208   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         268    269    +1     
  Branches       61     61           
=====================================
+ Hits          268    269    +1
Impacted Files Coverage Δ
src/downshift.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 23008b6...83646b6. Read the comment docs.

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 3, 2017

Ok, I've made a bunch of updates in the GitHub editor. We'll see if the build passes 😅

@divyenduz
Copy link
Collaborator Author

@divyenduz divyenduz commented Oct 3, 2017

I noticed that changes you made and they make sense to me. However, build fails because onOuterClick doesn't seem to be getting called. Do you have an idea of why that is happening?

You made the documentation look so much better 🥇

Also, let me know if I need to make edits to typings as well as suggested by @tansongyang. I noticed that onClick is also not defined in typings. I have no experience in TypeScript but this is pretty basic and I can do it.

Let me know what should I do next though.

Thanks

@@ -619,7 +619,7 @@ Thanks goes to these people ([emoji key][emojis]):
<!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->
| [<img src="https://avatars.githubusercontent.com/u/1500684?v=3" width="100px;"/><br /><sub>Kent C. Dodds</sub>](https://kentcdodds.com)<br />[💻](https://github.com/paypal/downshift/commits?author=kentcdodds "Code") [📖](https://github.com/paypal/downshift/commits?author=kentcdodds "Documentation") [🚇](#infra-kentcdodds "Infrastructure (Hosting, Build-Tools, etc)") [⚠️](https://github.com/paypal/downshift/commits?author=kentcdodds "Tests") | [<img src="https://avatars0.githubusercontent.com/u/100200?v=4" width="100px;"/><br /><sub>Ryan Florence</sub>](http://twitter.com/ryanflorence)<br />[🤔](#ideas-ryanflorence "Ideas, Planning, & Feedback") | [<img src="https://avatars3.githubusercontent.com/u/112170?v=4" width="100px;"/><br /><sub>Jared Forsyth</sub>](http://jaredforsyth.com)<br />[🤔](#ideas-jaredly "Ideas, Planning, & Feedback") [📖](https://github.com/paypal/downshift/commits?author=jaredly "Documentation") | [<img src="https://avatars1.githubusercontent.com/u/8162598?v=4" width="100px;"/><br /><sub>Jack Moore</sub>](https://github.com/jtmthf)<br />[💡](#example-jtmthf "Examples") | [<img src="https://avatars1.githubusercontent.com/u/2762082?v=4" width="100px;"/><br /><sub>Travis Arnold</sub>](http://travisrayarnold.com)<br />[💻](https://github.com/paypal/downshift/commits?author=souporserious "Code") [📖](https://github.com/paypal/downshift/commits?author=souporserious "Documentation") | [<img src="https://avatars0.githubusercontent.com/u/1045233?v=4" width="100px;"/><br /><sub>Marcy Sutton</sub>](http://marcysutton.com)<br />[🐛](https://github.com/paypal/downshift/issues?q=author%3Amarcysutton "Bug reports") [🤔](#ideas-marcysutton "Ideas, Planning, & Feedback") | [<img src="https://avatars2.githubusercontent.com/u/244704?v=4" width="100px;"/><br /><sub>Jeremy Gayed</sub>](http://www.jeremygayed.com)<br />[💡](#example-tizmagik "Examples") |
| :---: | :---: | :---: | :---: | :---: | :---: | :---: |
| [<img src="https://avatars3.githubusercontent.com/u/6270048?v=4" width="100px;"/><br /><sub>Haroen Viaene</sub>](https://haroen.me)<br />[💡](#example-Haroenv "Examples") | [<img src="https://avatars2.githubusercontent.com/u/15073300?v=4" width="100px;"/><br /><sub>monssef</sub>](https://github.com/rezof)<br />[💡](#example-rezof "Examples") | [<img src="https://avatars2.githubusercontent.com/u/5382443?v=4" width="100px;"/><br /><sub>Federico Zivolo</sub>](https://fezvrasta.github.io)<br />[📖](https://github.com/paypal/downshift/commits?author=FezVrasta "Documentation") | [<img src="https://avatars3.githubusercontent.com/u/746482?v=4" width="100px;"/><br /><sub>Divyendu Singh</sub>](https://divyendusingh.com)<br />[💡](#example-divyenduz "Examples") | [<img src="https://avatars1.githubusercontent.com/u/841955?v=4" width="100px;"/><br /><sub>Muhammad Salman</sub>](https://github.com/salmanmanekia)<br />[💻](https://github.com/paypal/downshift/commits?author=salmanmanekia "Code") | [<img src="https://avatars3.githubusercontent.com/u/10820159?v=4" width="100px;"/><br /><sub>João Alberto</sub>](https://twitter.com/psicotropidev)<br />[💻](https://github.com/paypal/downshift/commits?author=psicotropicos "Code") | [<img src="https://avatars0.githubusercontent.com/u/16327281?v=4" width="100px;"/><br /><sub>Bernard Lin</sub>](https://github.com/bernard-lin)<br />[💻](https://github.com/paypal/downshift/commits?author=bernard-lin "Code") [📖](https://github.com/paypal/downshift/commits?author=bernard-lin "Documentation") |
| [<img src="https://avatars3.githubusercontent.com/u/6270048?v=4" width="100px;"/><br /><sub>Haroen Viaene</sub>](https://haroen.me)<br />[💡](#example-Haroenv "Examples") | [<img src="https://avatars2.githubusercontent.com/u/15073300?v=4" width="100px;"/><br /><sub>monssef</sub>](https://github.com/rezof)<br />[💡](#example-rezof "Examples") | [<img src="https://avatars2.githubusercontent.com/u/5382443?v=4" width="100px;"/><br /><sub>Federico Zivolo</sub>](https://fezvrasta.github.io)<br />[📖](https://github.com/paypal/downshift/commits?author=FezVrasta "Documentation") | [<img src="https://avatars3.githubusercontent.com/u/746482?v=4" width="100px;"/><br /><sub>Divyendu Singh</sub>](https://divyendusingh.com)<br />[💡](#example-divyenduz "Examples") [💻](https://github.com/paypal/downshift/commits?author=divyenduz "Code") [📖](https://github.com/paypal/downshift/commits?author=divyenduz "Documentation") [⚠️](https://github.com/paypal/downshift/commits?author=divyenduz "Tests") | [<img src="https://avatars1.githubusercontent.com/u/841955?v=4" width="100px;"/><br /><sub>Muhammad Salman</sub>](https://github.com/salmanmanekia)<br />[💻](https://github.com/paypal/downshift/commits?author=salmanmanekia "Code") | [<img src="https://avatars3.githubusercontent.com/u/10820159?v=4" width="100px;"/><br /><sub>João Alberto</sub>](https://twitter.com/psicotropidev)<br />[💻](https://github.com/paypal/downshift/commits?author=psicotropicos "Code") | [<img src="https://avatars0.githubusercontent.com/u/16327281?v=4" width="100px;"/><br /><sub>Bernard Lin</sub>](https://github.com/bernard-lin)<br />[💻](https://github.com/paypal/downshift/commits?author=bernard-lin "Code") [📖](https://github.com/paypal/downshift/commits?author=bernard-lin "Documentation") |

This comment has been minimized.

@kentcdodds

kentcdodds Oct 3, 2017
Member

You're not going to want to edit this directly. It's automatically generated. If something needs to be updated, then update the .all-contributorsrc file :)

This comment has been minimized.

@divyenduz

divyenduz Oct 3, 2017
Author Collaborator

Noted, reverting and doing it via .all-contributorsrc 👍

This comment has been minimized.

@divyenduz

divyenduz Oct 3, 2017
Author Collaborator

This is now done via npm run add-contributor 👍

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 3, 2017

What's really odd is the test that's failing isn't the new one but this one and it's unclear to me how this simple change could have made that impact 🤔

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 3, 2017

Can you reproduce this breakage locally?

@divyenduz
Copy link
Collaborator Author

@divyenduz divyenduz commented Oct 3, 2017

Yes, I am able to reproduce the breakage locally as well. Here is a small snippet from my terminal on running npm run validate

[test:cover]  FAIL  src/__tests__/downshift.lifecycle.js[test:cover]   ● Console[test:cover][test:cover]     console.error node_modules/react-dom/cjs/react-dom.development.js:8305
[test:cover]       The above error occurred in the <Downshift> component:[test:cover]           in Downshift[test:cover]           in Unknown (created by WrapperComponent)
[test:cover]           in WrapperComponent
[test:cover]
[test:cover]       Consider adding an error boundary to your tree to customize error handling behavior.
[test:cover]       You can learn more about error boundaries at https://fb.me/react-error-boundaries.
[test:cover]
[test:cover]   ● handles mouse events properly to reset state
[test:cover]
[test:cover]     expect(jest.fn()).toHaveBeenCalledTimes(1)
[test:cover]
[test:cover]     Expected mock function to have been called one time, but it was called zero times.
@divyenduz
Copy link
Collaborator Author

@divyenduz divyenduz commented Oct 3, 2017

Reading code with ☕️ to pinpoint the issue 👍

@divyenduz
Copy link
Collaborator Author

@divyenduz divyenduz commented Oct 3, 2017

I observed that if I call reset like this - callback without parameters and wrapper.

this.reset(
          {type: Downshift.stateChangeTypes.mouseUp},
          this.props.onOuterClick,
        )

And remove the following from the new test case - since we are not passing any object after above change.

expect(onOuterClick).toHaveBeenCalledWith(
    expect.objectContaining({
      // just verify that it's the controller object
      isOpen: false,
      getItemProps: expect.any(Function),
    }),
  )

Everything falls into place.

So, I think this has to do with the way we are passing the callback with the object.
I tried various configurations to pass the object with bind, not using implicit return with arrow function but nothing worked thus far breaking a different set of test cases on different configurations.

Let me see if I can get around this in some way.

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 4, 2017

The problem is that there was no default for the onOuterClick. The way that reset works is it will default the given callback to an noop function if it's not provided, but since we provided one (() => this.props.onOuterClick()) that default wasn't being used and we tried to call a function that didn't exist. I'm not sure why it didn't give the full stacktrace though.

Should be fixed now though.

This PR is not ready to be merged yet. When merged, this will fix #206

This PR does the following (Will appreciate early feedback and comments):-

1. Add onOuterClick description to README. I can remove the link to a specific issue from there

2. Add onOuterClick to propTypes with type PropTypes.func

3. Call this.props.onOuterClick as a callback to reset function when it is called under onMouseUp event

4. Add a test case for newly added functionality. This is still incomplete. I started with the test case expecting it to fail in its current state where I open the menu and simulate a outside click with click on document.body. With the current state of the onOuterClick callback this should have resulted in isOpen to be false there by failing the test but it is not the case. Exploring.

Also, to simulate the click on document.body. I borrowed the function `mouseDownAndUp` from `downshift.lifecycle.js`. I can either move this test over there or expose that function in a better way than duplicating.

Let me know your feedback on the approach? Also, please point out if you believe that I understood the problem incorrectly.
Copy link
Member

@kentcdodds kentcdodds left a comment

This looks good to me 👍

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 4, 2017

Thanks a ton @divyenduz!

@kentcdodds kentcdodds merged commit b9f8cf8 into downshift-js:master Oct 4, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 4, 2017

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@divyenduz
Copy link
Collaborator Author

@divyenduz divyenduz commented Oct 4, 2017

Thanks @kentcdodds - It is an honor to join the team. Time to read most of the code and watch the screencasts you had made while creating downshift with some ☕️

@kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Oct 4, 2017

🎉

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

Successfully merging this pull request may close these issues.

4 participants