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

Refactor: app strings into msgid #1626

Merged
merged 9 commits into from
Aug 8, 2023
Merged

Refactor: app strings into msgid #1626

merged 9 commits into from
Aug 8, 2023

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Aug 7, 2023 •
edited
Loading

Closes #1571
Closes #1607

This is a giant PR, sorry 😳 Going commit-by-commit won't exactly help either, the first commit was huge.

Things to look for in review

  • Ensure there are no lingering msgid using our old style e.g. core.icons, eligibility.pages, enrollment.buttons
  • Confirm there is no broken copy UI - text should show up everywhere it is expected to
  • Confirm no regressions for Agency Selector Modal: Fix padding regression #1622 and Help: Mobile - Type fixes #1623
  • Confirm no regressions in link punctuation / spacing
  • bin/makemessages.sh should only update the timestamp
  • bin/init.sh should call compilemessages without error

@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. i18n Copy: Language files or Django i18n framework deployment-dev [auto] Changes that will trigger a deploy if merged to dev and removed back-end Django views, sessions, middleware, models, migrations etc. labels Aug 7, 2023
This was referenced Aug 8, 2023
Merged
Merged
@thekaveman thekaveman self-assigned this Aug 8, 2023
@thekaveman thekaveman modified the milestones: Veterans, Copy Improvements Aug 8, 2023
thekaveman
thekaveman commented Aug 8, 2023
View reviewed changes
benefits/core/templates/core/base.html
@@ -7,9 +7,13 @@
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="description" content="{% translate "core.pages.help.about.p[0]" %}">
{% blocktranslate trimmed asvar description %}
Copy link
Member Author

Choose a reason for hiding this comment

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

asvar is the syntax to store the results of a {% blocktranslate %} into a variable: https://docs.djangoproject.com/en/4.2/topics/i18n/translation/#std-templatetag-blocktranslate

Copy link
Member Author

Choose a reason for hiding this comment

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

trimmed removes extra whitespace in the translated block https://docs.djangoproject.com/en/4.2/topics/i18n/translation/#std-templatetag-blocktranslate

benefits/core/templates/core/help.html Outdated Show resolved Hide resolved
benefits/locale/en/LC_MESSAGES/django.po
#, python-format
msgid ""
"Cal-ITP doesn’t save any of your information. All MST transit benefits "
"reduce fares by 50%% for buses on fixed routes."
Copy link
Member Author

Choose a reason for hiding this comment

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

Double %% here needed to escape the ID, but gets formatted out of the final string.

benefits/locale/en/LC_MESSAGES/django.po
"Cal-ITP doesn’t save any of your information. All SacRT transit benefits "
"reduce fares by 50% for light rail."
"reduce fares by 50%% for light rail."
Copy link
Member Author

Choose a reason for hiding this comment

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

Double %% here needed to escape the ID, but gets formatted out of the final string.

@thekaveman thekaveman marked this pull request as ready for review August 8, 2023 20:55
@thekaveman thekaveman requested a review from a team as a code owner August 8, 2023 20:55
machikoyasuda
machikoyasuda requested changes Aug 8, 2023
View reviewed changes
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

Looks good overall. Added comments on some fixes (extra quotation mark, areas that need smart apostrophes). Agency Index CTA isn't working on my machine. Otherwise, looks good.

machikoyasuda
machikoyasuda approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@thekaveman thekaveman merged commit 119f9ce into dev Aug 8, 2023
10 checks passed
@thekaveman thekaveman deleted the refactor/msgid branch August 8, 2023 22:25
@angela-tran
Copy link
Member

🙌 Whoohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machikoyasuda machikoyasuda machikoyasuda approved these changes

Assignees

@thekaveman thekaveman

Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates i18n Copy: Language files or Django i18n framework migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
Milestone
Copy improvements
Development

Successfully merging this pull request may close these issues.

Make a new section on help page for Benefits source link Refactor msgids in PO files
3 participants
@thekaveman @angela-tran @machikoyasuda