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.

Sign up for GitHub

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

Jump to bottom

Fix broken translations in module list page #14613

Merged
merged 2 commits into from
Jul 17, 2019
Merged

Fix broken translations in module list page #14613

merged 2 commits into from
Jul 17, 2019

Conversation

matthieu-rolland
Copy link
Contributor

@matthieu-rolland matthieu-rolland commented Jul 12, 2019 •
edited by eternoendless
Loading

  • link module actions names to related translation domain
  • fix module action display name generation
  • use correct translation domain
Questions Answers
Branch? develop
Description? Translation broken in module listing page, in the action dropdown of each item.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14589
How to test? See description in the issue

This change is Reviewable

@matthieu-rolland
fix broken translation in module list page:
a0f2c1c 
- link module actions to related translation domain
- set those relations as module attribute
- fix module action display name generation
- use correct translation domain
@matthieu-rolland matthieu-rolland requested a review from a team as a code owner July 12, 2019 10:06
@prestonBot prestonBot added develop Branch Bug Type: Bug Waiting for wording Status: action required, waiting for wording labels Jul 12, 2019
matthieu-rolland
matthieu-rolland commented Jul 12, 2019
View reviewed changes
src/PrestaShopBundle/Resources/views/Admin/Module/Includes/action_button.html.twig
@@ -23,7 +23,7 @@
* International Registered Trademark & Property of PrestaShop SA
*#}

{% set displayAction = action|capitalize|replace({'_': " "})|trans({}, 'Admin.Actions') %}
{% set displayAction = action|title|replace({'_': " "})|trans({}, transDomain[action]) %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the capitalize filter would only capitalize the first character, here we need a ucwords kind of filter. Happily the title filter does just that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the trans filter be called before using the title one? Or else the key might not be recognized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question @jolelievre, initially the capitalize filter was used to build the translation key. But it failed, because capitalize makes a ucfirst, when what was needed to build a proper translation key was a ucwords (in this case anyway). The title filter does the needed ucwords

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok! then you need top check if the keys are present in the catalogue. Because the catalog is built by scanning the PrestaShop code and fetching all the hard coded strings. It is not able to compute variables. So unless these keys are used explicitly somewhere else in the code they won't be present in the catalogue and won't be translated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they exist in the catalog, from what I've seen and understood, those values are used as constants in the code (php side), and made to be transformed into translation keys easily (twig side).

@matthieu-rolland matthieu-rolland added the WIP Status: Work In Progress label Jul 12, 2019
@matthieu-rolland
Copy link
Contributor Author

matthieu-rolland commented Jul 12, 2019 •
edited
Loading

@LouiseBonnard
We need a translation for the word "browse" for when a button opens the file explorer.
(that would be "parcourir" in french)

PierreRambaud
PierreRambaud reviewed Jul 12, 2019
View reviewed changes
src/PrestaShopBundle/Resources/views/Admin/TwigTemplateForm/bootstrap_4_layout.html.twig Outdated
@@ -701,6 +701,11 @@
{% endblock birthday_widget %}

{% block file_widget %}
<style>
Copy link
Contributor

Choose a reason for hiding this comment

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

two spaces for indent please

@LouiseBonnard
Copy link
Contributor

Hello @breizoreol, thanks a lot for the PR! What translation do you need? Actually, the word Browse (localized in Admin.Actions) is okay, we just have to make it translatable - this way, it will be part of the 1.7.7 catalog on Crowdin and the community will be able to translate it in all different languages. Feel free to ask me more information if needed, perhaps my issue was not that clear. ;-)

LouiseBonnard
LouiseBonnard reviewed Jul 15, 2019
View reviewed changes
src/PrestaShopBundle/Resources/views/Admin/TwigTemplateForm/bootstrap_4_layout.html.twig Outdated
@@ -701,6 +701,11 @@
{% endblock birthday_widget %}

{% block file_widget %}
<style>
.custom-file-label:after {
content: "{{ "browse"|trans({}, 'Admin.Actions') }}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please write it Browse instead, with a capital letter at the beginning, thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done !

@LouiseBonnard
Copy link
Contributor

One question @breizoreol, how about the Disable mobile entry, would it be fixed in another PR or in this one?

@PierreRambaud PierreRambaud changed the title fix broken translation in module list page: Broken translation in module list page: Jul 17, 2019
@PierreRambaud PierreRambaud added Waiting for QA Status: action required, waiting for test feedback and removed WIP Status: Work In Progress labels Jul 17, 2019
@matthieu-rolland
Copy link
Contributor Author

One question @breizoreol, how about the Disable mobile entry, would it be fixed in another PR or in this one?

@LouiseBonnard yes, the 'Disable mobile' was not translated due to a mistake in the code, that is fixed in this pr.

@LouiseBonnard LouiseBonnard removed the Waiting for wording Status: action required, waiting for wording label Jul 17, 2019
@sarahdib sarahdib added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jul 17, 2019
@sarahdib sarahdib added this to the 1.7.7.0 milestone Jul 17, 2019
@matks
Copy link
Contributor

matks commented Jul 17, 2019

Thank you @matthieu-rolland 🎉

@matks matks merged commit 8598baa into PrestaShop:develop Jul 17, 2019
@matks matks mentioned this pull request Jul 17, 2019
Closed
mbadrani pushed a commit to mbadrani/PrestaShop that referenced this pull request Jul 18, 2019
@matks @mbadrani
Merge pull request PrestaShop#14613 from matthieu-rolland/fix-broken-…
2ab5e48 
…translation

Broken translation in module list page:
@eternoendless eternoendless changed the title Broken translation in module list page: Broken translation in module list page Aug 21, 2019
@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Aug 21, 2019
@eternoendless eternoendless changed the title Broken translation in module list page Fix broken translations in module list page Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jolelievre jolelievre jolelievre left review comments

@LouiseBonnard LouiseBonnard LouiseBonnard left review comments

@PierreRambaud PierreRambaud PierreRambaud approved these changes

Assignees
No one assigned
Labels
Bug Type: Bug develop Branch QA ✔️ Status: check done, code approved Waiting for wording Status: action required, waiting for wording
Projects
None yet
Milestone
1.7.7.0
Development

Successfully merging this pull request may close these issues.

Wording seems to be off translation
7 participants
@matthieu-rolland @LouiseBonnard @matks @PierreRambaud @jolelievre @sarahdib @prestonBot