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

Hoist non-static properties on withTheme HOC #712

Merged
merged 4 commits into from Apr 20, 2017
Merged

Conversation

kitten
Copy link
Member

@kitten kitten commented Apr 19, 2017

@kitten kitten added this to the v2.0 milestone Apr 19, 2017
@mxstbr-bot
Copy link

mxstbr-bot commented Apr 19, 2017

Warnings
⚠️ There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

Copy link
Member

@brunolemos brunolemos left a comment

Choose a reason for hiding this comment

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

@philpl Please don't do that (close another person PR and make it your own).
Instead, just request changes to the original contributor.


state: { theme?: ?Object } = {};
unsubscribe: () => void;
class WithTheme extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to do class WithTheme extends Component? Would that mean statics are available by simple prototype inheritance?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not a good idea, since we'd be overriding some original logic. I'd stick to the HOC-pattern 😉

@kitten
Copy link
Member Author

kitten commented Apr 20, 2017

@brunolemos I understand you're upset and I apologise for stepping on your toes. Please do understand however that I've thought about this and opened a fresh PR since it's going to be merged against v2 and now intersects/conflicts with #710. I'm currently trying to move quickly towards the v2 release, and I thought it's safe to move your changes over to a new branch.

I'll add an attribution with your name to the changelog, and I hope this doesn't scare you off from contributing to SC in the future. Sorry again! 😅

@kitten kitten merged commit 871463b into v2 Apr 20, 2017
@brunolemos
Copy link
Member

brunolemos commented Apr 20, 2017

@philpl I was just finishing to update this PR with the original commit plus yours, but nevermind, you merged it. Not good, philpl.

@kitten
Copy link
Member Author

kitten commented Apr 20, 2017

@brunolemos Hey, man. I'm sorry I went ahead, but I can't read your mind and guess what you're up to. Yes, this is unfortunate, but at the end of the day it's just code, so please don't be passive aggressive about this, because that doesn't help either of us, since it's not constructive.

I'm happy to buy you a beer and apologise in person, if we end up in the same city one day. We don't have to escalate this further and turn this into something bigger than it is. Sorry again, mate

@brunolemos
Copy link
Member

@philpl It's constructive in the way that my intention is that you don't do that again with other contributor or a project member like me. Besides that, no problem, keep moving fast.

@mxstbr mxstbr deleted the v2-hoist-static-hoc branch December 21, 2017 12:48
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