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 passTrough parameters to closePortal() #119

Closed
wants to merge 1 commit into from
Closed

add passTrough parameters to closePortal() #119

wants to merge 1 commit into from

Conversation

robinfehr
Copy link

@robinfehr robinfehr commented Dec 24, 2016

  • add pass trough params to the closePortal func
  • change file ending for jsx files, change webpack accordingly
  • fix tests
  • update babel-preset-es2015
  • add babel-preset-stage2 (nec. for named parameters and more)
  • fixing lint errors

- change file ending for jsx files, change webpack accordingly
- fix tests
- update babel-preset-es2015
- add babel-preset-stage2 (nec. for named parameters and more)
- fixing lint errors
@tajo
Copy link
Owner

tajo commented Dec 29, 2016

Reasons, examples, tests for add pass trough params to the closePortal func ?

change file ending for jsx files, change webpack accordingly

Why? I'm quite happy with using .js.

Also, you should probably split new features and things like update babel-preset-es2015 into different PRs.

This PR is not acceptable so I'm closing it. But feel free to reopen it with updates and comments.

Thank you!

@tajo tajo closed this Dec 29, 2016
@robinfehr
Copy link
Author

robinfehr commented Dec 30, 2016

hey @tajo - let's quickly discuss it and then I'll reopen them accordingly :)

my reason for the passTrough:
I'm currently implementing a drag and drop functionality. When for example a list entry gets dragged within the portal not only the portal has to be close but also the dragged entry details need to be passed to the onClose handler. Of course we could add an extra prop with another function but I'd much prefer to have it passed trough.

my reasons for jsx:
jsx is recommended by AirBnb guys (which we currently use) and some editors do not support a correct syntax highlighting without the .jsx file ending

about the splitting:
will do so.

what do you think? :)

@jdomizio
Copy link

There exist compelling (to me) arguments against the use of a jsx extension as well (facebook/create-react-app#87)

@robinfehr
Copy link
Author

Yeah fb against airbnb haha id say its a matter of taste in the end - lets leave it :)

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

3 participants