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

Hello world revision #584

Merged
merged 1 commit into from
Nov 19, 2016
Merged

Hello world revision #584

merged 1 commit into from
Nov 19, 2016

Conversation

mongjong59
Copy link
Contributor

@mongjong59 mongjong59 commented Oct 30, 2016

Fix #579

Revise some part of the hello world example with Redux


This change is Reviewable

@justin808
Copy link
Member

@robwise @jbhatab @Judahmeek I'd like to get your feedback on this one. I'm going to create a sample repo with a PR generated from the generator.

@justin808
Copy link
Member

@robwise @jbhatab @Judahmeek @nostophilia I created: shakacode/react_on_rails-test-new-redux-generation#1

Let's comment on the code there and here.

@justin808
Copy link
Member

@nostophilia

  1. Please see: Run generator redux updated with lint react_on_rails-test-new-redux-generation#2. We need to generate code that passes linting.
  2. We can cleanup the comments. We should remove the revision dates from the generated files. The more doc references we provide the better.

Overall, I think this looks great. Just needs a bit of cleanup!

Thanks for your contributions.

I'd like to get this merged in the next day or so. If you cannot get to it, please let me know. I'd prefer to have you make the changes so that your name is in the commit history with one big commit.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling 8175de8 on nostophilia:hello-world-revision into 4df498a on shakacode:master.

@mongjong59
Copy link
Contributor Author

@justin808 @robwise
I revised my code together with the syntax suggestions. Please let me know what else should I do before merging these commits into a single one. (I'm also in favor of leaving async middleware out as Rob mentioned. Maybe you guys can discuss this later)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling fb48387 on nostophilia:hello-world-revision into 4df498a on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling 3682d4f on nostophilia:hello-world-revision into 7fecce1 on shakacode:master.

@justin808
Copy link
Member

@nostophilia Please let me know when you're ready. Please confirm if you got all the requested changes and if linting passes using the linter 13.0.0.

npm i --save-dev [email protected] after running the generator.

@mongjong59
Copy link
Contributor Author

@justin808
It's ready except this violation

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling df77edb on nostophilia:hello-world-revision into 60d9f7f on shakacode:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling d8c1900 on nostophilia:hello-world-revision into cd2038d on shakacode:master.

@justin808
Copy link
Member

shakacode/react_on_rails-test-new-redux-generation#8

So close! Just need to fix the lint errors on what's generated.

BE SURE to use the linters in this commit:

shakacode/react_on_rails-test-new-redux-generation@ef1bde4

You can cherry-pick this!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling d815093 on nostophilia:hello-world-revision into 3a84802 on shakacode:master.


render() {
return (
<div>
<HelloWorldWidget name={this.state.name} updateName={e => this.updateName(e)} />
<HelloWorld name={this.state.name} updateName={this.updateName} />
Copy link
Contributor Author

@mongjong59 mongjong59 Nov 17, 2016

Choose a reason for hiding this comment

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

Please note that there will be a few small changes related to file and component name in no-redux example as HelloWorldWidget.jsx is shared.

Also, there's a lint violation here. And I simplified it a little bit with the arrow function.

@justin808
Copy link
Member

@nostophilia when will you push v6 to https://github.com/shakacode/react_on_rails-test-new-redux-generation/ ? I'd like to get this change into the next release coming maybe this weekend.

@justin808 justin808 modified the milestone: 6.2 Nov 19, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.987% when pulling e47256e on nostophilia:hello-world-revision into add8aa8 on shakacode:master.

@justin808
Copy link
Member

:lgtm_strong:

AMAZING JOB! Great teamwork! Big improvement!

I'm shipping this!


Reviewed 18 of 19 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

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.

3 participants