Allow custom partials in resource views

[jguiditta]

Ok, I feel like a pain for saying this, but I think this could be approached differently. Reading through the patch, I see lots of duplication, which always irks me. It may well be that each object view needs to needs to check for and then call the custom chunks, but I feel like we should be able to do something more like http://guides.rubyonrails.org/layouts_and_rendering.html#using-the-content_for-method. I am not sure if that works only in layouts (though I think we could still work with it if that were the case), but this could potentially chance all those altered views above to be:

  content_for?(:custom) ? yield(:custom)

And the host application can just include:

content_for :custom do

This should also allow you to just test the custom content in one place, as the current tests seem a bit of copy and paste. If it works in one place, it should work in all.

Now, if for some reason, there is absolutely no way to use the stuff built into rails to get what we need here, another way we could decrease the duplication is to extract those view snippets that check for 'custom' into the engine's application_helper and just make a simple call of

render_custom

or similar, and have the helper dynamically figure out the details it needs.

Lastly, I am going to merge in the rails upgrade branch into master now that all tests pass, so whatever changes you may make here, please rebase them and make the pull request point at master instead of the branch.

[mtaylor]

Hey Jay, thanks for the review.

So my understanding of content_for and yield is that is doesn't really do what we want in this scenario. It gives us control over what to render in a specific area of a layout; depending on which layout is rendered and what that layout has defined in the named tags.

However, it doesn't give us control over which specific layout to render. This is the job of the render method. So,if we did use content_for to position the content of our custom partial we would still have to explicitly render the partial ourself.

With regards to testing. Yes it is very copy and pastey but the purpose of these tests is not just to see if the approach works but also to highlight cases in the future when something brakes. If I only include a test for one example; to show that my chosen approach works; the tests would not catch any future breakages in views other than the one in the example test.

Pulling out the code that checks existence and renders of custom partial into application helper makes sense, there's no point duplicating that code. However, I will wait before I send an updated patch. Let's discuss the content_for and yield approach before I go any further. Maybe there is some way you envisage that I've not seen.

[jguiditta]

Ok, so, I tinkered with this a bit to no avail, I guess it doesn't work quite like I hoped. I am going to merge this as is, we can consolidate the calls later if you agree it makes sense to pull those into a helper.