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

Added property renderedHtml to return gen func #689

Merged

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Jan 18, 2017

This is to support React Router v4, which needs the result of calling
renderToString.


This change is Reviewable

@justin808
Copy link
Member Author

@robwise @alexfedoseev can I get a quick review on the JS parts?
@mapreal19 @samnang please take a quick peek at the Ruby code.

@coveralls
Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage remained the same at 99.321% when pulling b719cb6 on justin800/allow-router-result-to-return-html-string into 53af841 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.321% when pulling b719cb6 on justin800/allow-router-result-to-return-html-string into 53af841 on master.

@coveralls
Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage remained the same at 99.321% when pulling b719cb6 on justin800/allow-router-result-to-return-html-string into 53af841 on master.

@coveralls
Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage increased (+0.008%) to 99.329% when pulling 0512a2b on justin800/allow-router-result-to-return-html-string into 53af841 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 99.329% when pulling 0512a2b on justin800/allow-router-result-to-return-html-string into 53af841 on master.

@@ -176,6 +176,15 @@ def change_text_expect_dom_selector(dom_selector)
end
end

feature "renderedHtml from generator function", :js do
subject { page }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you prefer to call it subject instead of page? To me, page is more explicit and specific in this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm being consistent with other tests in this file.

background { visit "/rendered_html" }
scenario "renderedHtml should not have any errors" do
expect(subject).to have_text "Props: {\"hello\":\"world\"}"
expect(subject.html).to include("[SERVER] RENDERED RenderedHtml to dom node with id")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to check with the whole string here instead of putting assertion on what we want to verify each part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this is in a script tag.

This is to support React Router v4, which needs the result of calling
renderToString.
@justin808 justin808 force-pushed the justin800/allow-router-result-to-return-html-string branch from e04231c to 4cf969a Compare January 18, 2017 08:40
@mapreal19
Copy link
Member

Reviewed 6 of 19 files at r1.
Review status: 6 of 18 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@coveralls
Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage remained the same at 99.329% when pulling 4cf969a on justin800/allow-router-result-to-return-html-string into 4e567a3 on master.

@coveralls
Copy link

coveralls commented Jan 18, 2017

Coverage Status

Coverage remained the same at 99.329% when pulling 4cf969a on justin800/allow-router-result-to-return-html-string into 4e567a3 on master.

@alex35mil
Copy link
Member

@justin808 I tried only client part of react-router@4 yet. Looking at docs for server rendering, api seems kinda strange, I don't get why 404 case requires second render. But anyway, talking about integration w/ react_on_rails— it LGTM.

@justin808 justin808 merged commit c5e968d into master Jan 27, 2017
@justin808 justin808 deleted the justin800/allow-router-result-to-return-html-string branch January 27, 2017 08:39
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.

5 participants