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

Add render prop to Conditional #21

Merged
merged 1 commit into from
Jan 21, 2018
Merged

Add render prop to Conditional #21

merged 1 commit into from
Jan 21, 2018

Conversation

ferdinandsalis
Copy link
Contributor

@ferdinandsalis ferdinandsalis commented Jan 18, 2018

Here is my first stab at adding tests and some documentation. I am sure you are still making a few changes for the release, so feel free to discard the pull request or make changes.

@MicheleBertoli
Copy link
Owner

This is awesome, thank you very much @ferdinandsalis.

I've got just a few questions/considerations but it looks great to me:

  • why are we passing { visibile } instead of just visibile? (this is the only thing that prevented me from pressing "merge" straight away : )).
  • I suggest we use render props rather than "function as child".
  • it's might worth reorganizing the tests and separate the callbacks from the visible (children / render props) and not visible states - I/we can do it later.

@ferdinandsalis
Copy link
Contributor Author

  • Do we ever need to pass in any other data, if not I guess we could just pass the boolean
  • Will do. Should we maybe allow both?
  • Yes I thought so too. However I am not that apt yet when it comes to testing. Will try, maybe …

Thanks for the feedback!

@MicheleBertoli
Copy link
Owner

  • I would go for the boolean approach, and if we ever need more data we can pass multiple params (unless they become too many :))
  • Although I tend to prefer the "function as child" approach, I would go with the render prop (as it seems more common, and it's used in the official React docs)
  • Let's consider this a nice to have - I'll be happy to merge and refactor the tests later

Thank you very much one more time.

@ferdinandsalis ferdinandsalis changed the title Expose visible when passing function as children to Action and State Add render prop to Conditional Jan 20, 2018
@MicheleBertoli
Copy link
Owner

🙌

@MicheleBertoli MicheleBertoli merged commit 6a8307a into MicheleBertoli:master Jan 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants