search

ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.7k stars 244 forks source link

Merge master into feature/redux-opinionated #471

Closed SleepWalker closed 6 years ago

SleepWalker commented 7 years ago

Hello guys,

I've merged feature/redux-opinionated with master and I think it should be updated on github too, to simplify the life for other devs

sergiokopplin commented 7 years ago

@SleepWalker i think it does the same as this one: https://github.com/ctrlplusb/react-universally/pull/366

SleepWalker commented 7 years ago

@sergiokopplin oh, sorry. I can't understand how does it happend, but this pr is a result of merging feature/redux-opionated into master made today 0o

and when I do something like this: https://github.com/ctrlplusb/react-universally/compare/master...feature/redux-opinionated https://github.com/ctrlplusb/react-universally/compare/ctrlplusb:feature/redux-opinionated...SleepWalker:feature/redux-opinionated https://github.com/ctrlplusb/react-universally/compare/master...SleepWalker:feature/redux-opinionated

I see really strange results. Current feature/redux-opinionated can not be merged into master without conflicts and the diff looks strange. Probably, I'm doing something wrong and if so, then this PR should be closed. But I can't really understand whats wrong

sergiokopplin commented 7 years ago

hey @SleepWalker, it might be because i did an old pr with the next branch, but you're right, it's behind the master now, i think we can go ahead with this one!

SleepWalker commented 7 years ago

merged with current master

sergiokopplin commented 7 years ago

Hey @SleepWalker, i've done the same work here https://github.com/ctrlplusb/react-universally/pull/489, but it's closed in favor of yours. Duplicated PR!

Can you check if the PR's has the same changes? I've updates some dependencies and routes. Cheers!

oyeanuj commented 7 years ago

@SleepWalker Is this ready to merge?

SleepWalker commented 7 years ago

@oyeanuj yep. @sergiokopplin I've applied updates from you PR, but some of them was erased by precommit hook. Regarding Counter route — I leaved it as is, because this route is from master branch. I think it would be better to touch master code as less as possible

sergiokopplin commented 7 years ago

@SleepWalker yeah, you're right! Any other specific code was removed?

SleepWalker commented 7 years ago

Removed from where? The last commit contains all the meaningful diff from your's PR except almost all codestyle improvements (some of them I've reject and the biggest part was erased by precommit hook :( )

So, as you see, our PRs was almost the same...

sergiokopplin commented 7 years ago

Anywhere actually, but it seems ok as you said. 👍

oyeanuj commented 7 years ago

@sergiokopplin @birkir Can this be merged then?

sergiokopplin commented 7 years ago

I think so! Have you tested it locally @oyeanuj?

oyeanuj commented 7 years ago

@sergiokopplin I haven't tested much yet. I was assuming it was tested by others :) I was hoping that once this is merged in, I could then test it together with my existing setup.

But I understand if you want others to test or bless it first.

oyeanuj commented 6 years ago

@sergiokopplin I can confirm that it works well locally, so, I'd say merge if it tests well for you.

sergiokopplin commented 6 years ago

Let's merge then! @oyeanuj @SleepWalker @ctrlplusb, let's do that?

SleepWalker commented 6 years ago

@sergiokopplin yep, let's do that

ctrlplusb commented 6 years ago

Sounds good to me. 😀