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

Small refactorings #1068

Merged
merged 1 commit into from
Apr 24, 2018
Merged

Small refactorings #1068

merged 1 commit into from
Apr 24, 2018

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Apr 23, 2018

This change is Reviewable

@justin808 justin808 force-pushed the small-refactorings branch from 3163d84 to 95e319c Compare April 24, 2018 01:27
@justin808
Copy link
Member Author

Lots of better names...


Review status: 0 of 11 files reviewed at latest revision, all discussions resolved, some commit checks broke.


lib/react_on_rails/react_on_rails_helper.rb, line 101 at r1 (raw file):

    #      if the JS code throws
    # Any other options are passed to the content tag, including the id.
    def react_component(component_name, options = {})

options is for a plain Hash

renderer_options is for our special object of configuration when rendering


Comments from Reviewable

- `react_component` allows logging_on_server specified at the component level.
- Missing class when throwing some error messages.
@justin808 justin808 force-pushed the small-refactorings branch from 95e319c to e224450 Compare April 24, 2018 01:45
@justin808
Copy link
Member Author

@mapreal19 Still review.

@justin808 justin808 merged commit 57fdc40 into master Apr 24, 2018
@justin808 justin808 deleted the small-refactorings branch April 24, 2018 02:09
@coveralls
Copy link

coveralls commented Apr 24, 2018

Coverage Status

Coverage remained the same at ?% when pulling e224450 on small-refactorings into ede0ac8 on master.

@mapreal19
Copy link
Member

:lgtm: just a couple of minor things


Reviewed 9 of 15 files at r1, 2 of 4 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


lib/react_on_rails/react_on_rails_helper.rb, line 435 at r2 (raw file):

        result = ReactOnRails::ServerRenderingPool.server_render_js_with_console_logging(js_code, render_options)
      rescue StandardError => err
        # This error came from the renderer

I'd remove this comment. The reader can see we're raising a prerender error anyway


lib/react_on_rails/server_rendering_pool/ruby_embedded_java_script.rb, line 62 at r2 (raw file):

      class << self
        private

we should leave private here?


Comments from Reviewable

@mapreal19 mapreal19 self-assigned this Apr 24, 2018
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.

3 participants