Skip to content

Upgrade to support React 16 #80

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

Closed

Conversation

paularmstrong
Copy link
Contributor

No description provided.

@paularmstrong
Copy link
Contributor Author

ping @rxaviers

@rxaviers
Copy link
Member

Hi @paularmstrong, the content of these changes LGTM, but we should give credits to the original author (i.e., #76). I encourage we do something analogous to #79, where I preserved authorship in 8d92b0f and dd35eb8. Cherry-picking the original changes is the main responsible for the delay on my end. Help appreciated. Thanks

@paularmstrong
Copy link
Contributor Author

@diligiant do you care? This seems like a waste of time and resources.

@diligiant
Copy link

I agree with @paularmstrong : why bother when you can slow down everyone for nothing and later get the credit for doing the work?

@rxaviers
Copy link
Member

@paularmstrong, as you can see from @diligiant's response above, the "waste of time" is debatable. Note #76 already had the implementations you are redoing/splitting here plus in #79, but there were arguments against merging it, because it was implementing too many things at once and it was ideal to split it. I agreed with such argument despite the fact it slows down the process. Here we are. Now, I cannot ignore the original authorship either.

The resources in this open source project are pretty scarce, I agree. You (@paularmstrong) and @diligiant are bringing valuable contributions to this project and I really wanted us to converge efforts. I am trying to accommodate both interests for that reason.

@diligiant
Copy link

diligiant commented Oct 11, 2017 via email

@rxaviers
Copy link
Member

I acknowledge that in an attempt to accommodate both, I risk accommodating none. 😞

@necolas
Copy link
Contributor

necolas commented Oct 13, 2017

Please let's merge this and move on

rxaviers pushed a commit that referenced this pull request Oct 17, 2017
Ref #76
Ref #80

Signed-off-by: Frédéric Miserey <[email protected]>
rxaviers pushed a commit that referenced this pull request Oct 17, 2017
rxaviers pushed a commit that referenced this pull request Oct 17, 2017
Ref #76
Ref #80

Signed-off-by: Frédéric Miserey <[email protected]>
rxaviers pushed a commit that referenced this pull request Oct 17, 2017
@rxaviers
Copy link
Member

Thank you @diligiant and @paularmstrong. This has been merged by 457efe2 and 163fbd1.

@rxaviers rxaviers closed this Oct 17, 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
Development

Successfully merging this pull request may close these issues.

4 participants