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

Patch Rails 5.1 not supporting text option for render anymore #519

Closed

Conversation

weggesser
Copy link
Contributor

Ran into an exception while trying to use this with rails 5.1 - :text option is no longer supported by ActionView:: TemplateRenderer

begin
render text: template
rescue ArgumentError
render body: template
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify this block and use body as the argument no matter what?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like render accepts an :html option, should we use that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :html would prob. be better - :body would be the closest equivalent to what :text does/did and I was not sure if the :html option was unwanted in some cases ... For compatibility I chose to have this rescue block - knowing that it's kind of ugly :/
4.0 seems not to support :body and :html options for render. http://guides.rubyonrails.org/v4.0/layouts_and_rendering.html#using-render

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying.

This project supports rails>=4.1, so a breaking change in 4.0 is not a show stopper.

@wireframe
Copy link
Contributor

@seanpdoyle any blockers to merging this in? Rails 5.1 has hit release candidate and it would be awesome to have a ember-cli-rails release w/ compatibility on day one if possible.

@seanpdoyle
Copy link
Contributor

@wireframe CI is failing, but it looks like it might be unrelated.

Once it's green, we can merge.

@seanpdoyle
Copy link
Contributor

Unfortunately, I've been unable to correct the issues with CI failing.

I've tested this change manually and it's good to go, and has been merged in 585cfea.

@seanpdoyle seanpdoyle closed this May 12, 2017
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