Allows displaying information by Theme below text area in followup

[robvandijk]

What does this do?

It allows a Theme to display information below the text area in followup

Why was this needed?

We need to inform the user about the option to pass a telephone number for contact information

Implementation notes

We discussed this today by email. My initial thought was to extract a part of the view into a partial, and then to make adjustments to that smaller partial (smaller templates are better for maintenance purposes). Then I realized that here (and in other upcoming cases) I just want to add information, not to change any existing parts. Therefore I created this PR using a kind of plug-in mechanism for themes to be able to add info at certain (important) locations.
If you don't like this approach I can change this PR to the earlier discussed solution by extracting smaller partials (although that will require some more maintenance in the future).

---

Have you updated the changelog? If this is not necessary, put square brackets around this: [skip changelog]

[robvandijk]

BTW I have to add info at more locations. I will add these to this PR, once I've heard from you which solution you prefer (checking if partials supplied by themes exist, or splitting views into smaller partials to be overridden by themes).

[garethrees]

Our usual way of providing theme hooks is a blank partial (example), though I'm not adverse to adding hooks like this, as it avoids a blank template in core. If we went down this approach it would be nice to add a helper so we could do something like:

<%= render_theme_partial 'request/name_of_partial' %>

In this case though its not obvious what the proposed template is meant to do. Its name ("theme_info_for_requester") is pretty generic and is in the wrong folder for this view (should be in views/followups rather than views/request). I'd call this more like followups/form/followup_message_help.

[robvandijk]

Our usual way of providing theme hooks is a blank partial (example), though I'm not adverse to adding hooks like this, as it avoids a blank template in core. If we went down this approach it would be nice to add a helper so we could do something like:

<%= render_theme_partial 'request/name_of_partial' %>

In this case though its not obvious what the proposed template is meant to do. Its name ("theme_info_for_requester") is pretty generic and is in the wrong folder for this view (should be in views/followups rather than views/request). I'd call this more like followups/form/followup_message_help.

It was a quick example of the direction I was thinking of, and far from perfect. Thanks for the suggestions. I added helper render_theme_partial and included it in various locations. For the names of the partials I decided to stay as neutral as possible and refrained from using terms such as help: other Themes may want to use such a partial for different reasons. So I decided to stick to the location where render_theme_partial is used (e.g. below_...).

I have also created a PR for the Woo-knop theme, see mysociety/wob-knop-theme#47. It depends on the render_theme_partial helper introduced in this PR.

[garethrees]

Thanks for the updates. I've made a couple of naming suggestions with a view to what would make it easier for longer term maintenance, and I think a couple of the render_theme_partial calls could be removed where the main partial is small enough to just override in its entirity.

After these updates I think this would be good to merge, so once you've made them just squash it all in to a single commit.

[garethrees]

This should be prefixed alaveteli_pro/info_request_batches. I don't think we should assume that the contents of these will definitely be the same across e.g. request, pro request, batch request, etc.

I'd actually nest this under form, too, to make it super clear its part of the form (rather than e.g. a view/preview step.

Overall I'd make this alaveteli_pro/info_request_batches/form/below_request_body_field.

[garethrees]

Similar to the comment above, I'd make this alaveteli_pro/info_requests/form/below_request_body_field.

[garethrees]

I'd call this followups/preview/below_followup_message

[garethrees]

Would call this request/preview/below_request

[garethrees]

Similar to the comment above, I'd make this followups/form/below_request_body_field.

In this case we haven't extracted the form to a partial, so the namespacing doesn't quite match reality, but I think we ought to extract the form (which can be done later) so might as well get this piece right now.

[garethrees]

I'd call this request/form/below_request_body_field.

[robvandijk]

Thanks for the updates. I've made a couple of naming suggestions with a view to what would make it easier for longer term maintenance, and I think a couple of the render_theme_partial calls could be removed where the main partial is small enough to just override in its entirity.

After these updates I think this would be good to merge, so once you've made them just squash it all in to a single commit.

Thanks Gareth for the quick response. Will get back to this after my holidays. Will process the suggested naming changes. I'm wondering about removing some of the render_theme_partials and instead overwrite partials though. The method of overwriting partials, however small, is likely to cause more future maintenance than using the helper. Perhaps we can have a call to discuss this more?

[garethrees]

I'm wondering about removing some of the render_theme_partials and instead overwrite partials though. The method of overwriting partials, however small, is likely to cause more future maintenance than using the helper.

I think there's a tradeoff between maintenance of core vs maintenance of the theme here. I think we want to balance this – and I completely appreciate its not a good balance right now – but more render_theme_partials would mean more maintenance on our side to ensure they don't disappear underneath theme maintainers if/when we need to rename/remove them.

I also want to be a bit judicious about where they're included, especially as its a new idea that may or may not work out in the long run. If taken way too far we could end up with them before, in the middle of and after nearly every template!

Having come back and looked at this today, I think it does feel like even some of the ones I didn't initially suggest to be overrides take the idea a bit far – request/form/below_request_body_field for example is very specific and lots of includes like this could get out of hand.

Happy to jump on a call to hash out some heuristics for when an override/include hook is most appropriate.

[robvandijk]

Processed most of the proposed changes, except for the below_request and below_followup ones - let's discuss in the call how to deal with those.