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

Translate emails #10914

Merged
merged 7 commits into from
Dec 11, 2019
Merged

Translate emails #10914

merged 7 commits into from
Dec 11, 2019

Conversation

GSadee
Copy link
Member

@GSadee GSadee commented Dec 10, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
License MIT

Order confirmation in e.q. pl_PL:
Zrzut ekranu 2019-12-10 o 10 47 02

@GSadee GSadee added the UX Issues and PRs aimed at improving User eXperience. label Dec 10, 2019
@GSadee GSadee requested a review from a team as a code owner December 10, 2019 10:21
@@ -1,29 +1,29 @@
{% extends '@SyliusShop/Email/layout.html.twig' %}

{% block subject %}
Order confirmation
{{ 'sylius.email.order_confirmation.subject'|trans({}, null, localeCode) }}
Copy link
Member Author

@GSadee GSadee Dec 10, 2019

Choose a reason for hiding this comment

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

This could also be written as:

{% trans into localeCode %}sylius.email.order_confirmation.subject{% endtrans %}

What is a better option? /cc @Sylius/core-team

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the {% trans into localeCode %} version when no parameters or domain is present. It looks cleaner :D

Copy link
Contributor

Choose a reason for hiding this comment

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

But it would be nice to create a PR for twig that adds a trans_default_locale just as they have a trans_default_domain. That would really help with the readability of such templates as this

@vvasiloi
Copy link
Contributor

I find passing the locale code to every translation call very annoying, so I did this:
https://gist.github.com/vvasiloi/40ed49e336fd4be22544e3e9e1c81e9a

@GSadee
Copy link
Member Author

GSadee commented Dec 10, 2019

@vvasiloi Thanks for the suggestion, I was thinking about it and your solution looks quite convincing 👍

@GSadee GSadee changed the title [WIP] Translate emails Translate emails Dec 11, 2019
public function anEmailWithResetTokenShouldBeSentTo(string $recipient, string $localeCode = 'en_US'): void
{
$this->assertEmailContainsMessageTo(
$this->translator->trans('sylius.email.password_reset.reset_your_password', [], null, $localeCode),
Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of this solution, but @pamil had some second thoughts about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't have any better ideas though :)

@pamil pamil merged commit 5aa00a7 into Sylius:master Dec 11, 2019
@pamil
Copy link
Contributor

pamil commented Dec 11, 2019

Thanks, Grzegorz! 🎉

@GSadee GSadee deleted the translatable-emails branch December 11, 2019 12:10
@acornforth
Copy link
Contributor

I realise this feature is now added in 1.7. and I'm sure my question has been asked before, but why are emails not implemented as translatable entities?
Editing email content is often a role performed by content managers or admin staff. it seems counter-intuitive to require developer access and edit yaml files for this task...

I realise there would be BC concerns with this approach, but in principle I think it could be a more versatile approach overall.... maybe something to think about for Sylius 2.0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UX Issues and PRs aimed at improving User eXperience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants