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.

Sign up for GitHub

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

Jump to bottom

ReactOnRails version 10 #951

Merged
merged 14 commits into from
Oct 17, 2017
Merged

ReactOnRails version 10 #951

merged 14 commits into from
Oct 17, 2017

Conversation

Judahmeek
Copy link
Contributor

@Judahmeek Judahmeek commented Oct 6, 2017 •
edited by justin808
Loading

This change is Reviewable

@justin808
Copy link
Member

LGTM so far.

  1. Doc changes?
  2. Changes to spec/dummy and other tests to use the new API?

Reviewed 25 of 25 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

app/helpers/react_on_rails_helper.rb, line 324 at r1 (raw file):

  end

  def internal_react_component(component_name, raw_options = {})

should this be inside "private"

Comments from Reviewable

@Judahmeek Judahmeek force-pushed the jmeek/version-10 branch 2 times, most recently from 694ed18 to 185d6cc Compare October 7, 2017 08:17
@Judahmeek Judahmeek force-pushed the jmeek/version-10 branch 15 times, most recently from b673e6e to 8f58874 Compare October 13, 2017 10:35
@justin808
Copy link
Member

:lgtm:

Reviewed 19 of 19 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

@justin808 justin808 merged commit 0e06461 into master Oct 17, 2017
shepmaster
shepmaster reviewed Oct 18, 2017
View reviewed changes
package.json
@@ -87,7 +88,7 @@
},
"homepage": "https://github.com/shakacode/react_on_rails#readme",
"dependencies": {
"react-on-rails": "^9.0.0-beta.12",
"react-on-rails": "^9.0.3",
Copy link

Choose a reason for hiding this comment

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

Why does React on Rails version 10 require React on Rails version 9?

Copy link
Member

Choose a reason for hiding this comment

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

@Judahmeek This should be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

@shepmaster Until we release, we can't update the version in the PR.

Copy link

Choose a reason for hiding this comment

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

Until we release

I noticed it because the 10.0 release of this package requires the 9.0 release of this package 😸

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems [email protected] is released.
I send a PR to update this version. #959

@Judahmeek Judahmeek deleted the jmeek/version-10 branch October 19, 2017 23:32
@Judahmeek Judahmeek mentioned this pull request Oct 20, 2017
Merged
justin808 pushed a commit that referenced this pull request Oct 24, 2017
@Judahmeek @justin808
Add missed react_component_hash functionality (#960)
c1bce8f 
When I created #951, I forgot to add the useful functionality from #950 that would ensure that react_component_hash worked correctly. When there is a prerendering error, react_component_hash should return the result combined with the console logs as a hash even though every prerendering error is returned from Exec as a String. Right now, if a prerendering error occurs, react_component_hash throws its "Expected a Hash" error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shepmaster shepmaster shepmaster left review comments

@tricknotes tricknotes tricknotes left review comments

@justin808 justin808 justin808 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Judahmeek @justin808 @shepmaster @tricknotes