- Issue created by @Anybody
- 🇩🇪Germany Anybody Porta Westfalica
Would be great to have short maintainer feedback, if they'd like this functionality to be implemented, then we'll prepare it. We have two customers interested.
- 🇮🇱Israel jsacksick
Valid feature request IMO, one potential "problem" I can think of would be translability, but config entities are translatables, so it'd be a matter of ensuring the translated header/footer is injected into the receipt.
- 🇩🇪Germany Anybody Porta Westfalica
Thank you for the feedback @jsacksick! So we'll start implementing this.
Yes, this needs to be passed to the template in order language. But I think that should be solvable, hopefully. The other already existing content of the receipt already need the correct language for all contents. As there's already an implementation for the subject, that's either solved already or not yet solved. I hope for the first! :)
Header + Footer is alright?
I was thinking about a token / placeholder based "body" field, but I think header and footer is more bulletproof, good enough and what the store owners need around the table of order items etc.Furthermore, I think if that's not enough, it's valid to have a developer or themer for more custom stuff.
Once you give us green light, we'll implement this! :)
- 🇮🇱Israel jsacksick
Only thing is... Not sure it is "easy" to understand where the text is going to be output from the admin interface context.
Also for example, in the footer there is:
{{ 'Thank you for your order!'|t }}
, once we implement this, we should either skip this if the footer is not empty, or we could set this as the footer text the default order type provided.
Not sure what is the best solution. - 🇩🇪Germany Anybody Porta Westfalica
Yeah, I think we should move it into the footer field, it's a good example and customizable then!
For existing installations, I'd use an update hook to inform the users about the change and possible requirement to update their theme override.I think it would be best to have this in 3.x as it's kind of (little) breaking change due to the new template variables. Fine with the new major release then, but of course we'll do our best to keep the change as minimal as possible.
- Assigned to lrwebks
- Status changed to Needs work
3 months ago 12:27pm 29 August 2024 - 🇩🇪Germany Anybody Porta Westfalica
Tests fail with: "Undefined array key "Your order has been placed.""
Also please make this a non-draft, seem only the creator can do that. :)
- 🇮🇱Israel jsacksick
Also please make this a non-draft, seem only the creator can do that. :)
What difference does it make? Seems the tests are still running? Also because the tests are failing, this is technically not ready.
- 🇩🇪Germany Anybody Porta Westfalica
What difference does it make? Seems the tests are still running? Also because the tests are failing, this is technically not ready.
In the past I think the tests were not executed automatically, when this was a draft?
The reason mainly was, that this is blocked as "Draft" to the person / creator who set that flag. I think we should focus on the Drupal.org issue status mainly, until it's all GitLab. I didn't say draft is wrong :)
- 🇩🇪Germany Anybody Porta Westfalica
I left some comments on the MR, maybe @jsacksick also would like to take a look? (And see if he disagrees somewhere or has a stronger opinion)
- 🇩🇪Germany Anybody Porta Westfalica
@lrwebks further tasks:
- To come closer to the reasons for the failing test, please create a dedicated test with custom header and footer text set in config
- Update hook is missing for existing sites.
- Status changed to Needs review
3 months ago 10:33am 4 September 2024 - Issue was unassigned.
- 🇩🇪Germany Anybody Porta Westfalica
Okay the last test error is really crazy. It fails now, because the french text for the payment method "Paiement à la livraison" is now line-breaking. But we didn't touch it at all.
- Assigned to Anybody
- 🇩🇪Germany Anybody Porta Westfalica
Perhaps it would make sense to use
{% apply spaceless %}{% endapply %}
(https://symfony.com/blog/better-white-space-control-in-twig-templates) in the mail template to clean up the whitespaces in general for the text-only result?Or we would need to remove
\n
's from the tests?I guess this is caused, because the MR fixed the wrong indentation in that file and that's just the one space too much for it.
- Issue was unassigned.
- 🇩🇪Germany Anybody Porta Westfalica
Ok that's it... the spaces messed up the text-only version.
If @jsacksick is fine with that, we can either fix it here in combination or do the clean-up in a separate issue we merge before.
Seems to me as if it's generally useful to remove whitespace in twig email templates. I think I remember a similar case from the past in a different module.
- Status changed to RTBC
2 months ago 9:38am 12 September 2024 Works great, RTBC from my side, ready for maintainer review and feedback as of #18!