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

Fix i18n in HTML Files, Part Two #9025

Merged
merged 9 commits into from
Apr 24, 2024

Conversation

merwhite11
Copy link
Contributor

Closes #8904

Refactor. Added i18n syntax for value, title and alt attributes in remaining HTML files.

Technical

Ran extraction command, no failures, messages.pot file updated.

Testing

General
docker compose run web make test
docker compose run web pytest openlibrary/plugins/importapi/tests/test_import_validator.py

To extract syntaxed messages:

docker compose build home (to incorporate updates to docker file)
docker compose run --rm -uroot home ./scripts/i18n-messages extract

Screenshot

Stakeholders

@rebecca-shoptaw
@cdrini

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 15.95%. Comparing base (44e250f) to head (26170f3).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9025   +/-   ##
=======================================
  Coverage   15.95%   15.95%           
=======================================
  Files          89       89           
  Lines        4712     4712           
  Branches      822      822           
=======================================
  Hits          752      752           
  Misses       3450     3450           
  Partials      510      510           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cdrini cdrini self-assigned this Apr 4, 2024
package.json Outdated
@@ -119,6 +119,8 @@
"collectCoverage": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh these snuck in and shouldn't be here! You can revert them back by doing something like:

# First update your master branch
# then,
git checkout master package-lock.json
git checkout master package.json

This should take the version of these files currently on master, effectively "undoing" the changes in your branch! Save these on a separate commit 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated this to follow this syntax: openlibrary/templates/books/daisy.html
But neither this message or the one from daisy.html are making it to messages.pot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is valid. Modeled after openlibrary/templates/books/daisy.html and neither messages are showing up in messages.pot

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm that's odd! Can you create a new issue for this? There might be a bug preventing certain files from being read!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrini
I checked the pot file and the 'from You' seems to be extracting fine now that it's not an if/else --

msgid "from <a href=\"$list.owner.key\">%(owner)s"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sweet! Woohoo closed an issue before it was even opened! 😁 :success-kid:

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Nice looks good! A few small fixes 👍

openlibrary/templates/books/mobile_carousel.html Outdated Show resolved Hide resolved
openlibrary/templates/lists/list_overview.html Outdated Show resolved Hide resolved
openlibrary/templates/lists/list_overview.html Outdated Show resolved Hide resolved
@@ -19,7 +19,7 @@ <h3>
$if list.description:
$:sanitize(format(list.description))
$else:
<p>$_('No description.')</p>
<p>$_("No description.")</p>
Copy link
Collaborator

@cdrini cdrini Apr 24, 2024

Choose a reason for hiding this comment

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

Note: Like we mentioned in that comment let's not change these going forward to avoid unnecessary conflicts, but I know these changes happened before we had that discussion, so it's not worth our time to change this. Just a note for the future!

@cdrini
Copy link
Collaborator

cdrini commented Apr 24, 2024

Ok I also cleaned up those package*.json files:

$ git fetch upstream master
$ git checkout upstream/master package-lock.json
Updated 1 path from fd852cf6f
$ gitpod /workspace/openlibrary (#8904-Second-Batch) $ git checkout upstream/master package.json
Updated 1 path from fd852cf6f
$ git status
On branch #8904-Second-Batch
Your branch is up to date with 'origin/#8904-Second-Batch'.

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        modified:   package-lock.json
        modified:   package.json

$ git commit -m "Revert unintentional changes to package*.json files"
[#8904-Second-Batch cc6598a8f] Revert unintentional changes to package*.json files
 2 files changed, 2 insertions(+), 2 deletions(-)
$ git push origin HEAD
Enumerating objects: 7, done.
Counting objects: 100% (7/7), done.
Delta compression using up to 16 threads
Compressing objects: 100% (4/4), done.
Writing objects: 100% (4/4), 427 bytes | 427.00 KiB/s, done.
Total 4 (delta 3), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (3/3), completed with 3 local objects.
To https://github.com/merwhite11/openlibrary.git
   80002ad13..cc6598a8f  HEAD -> #8904-Second-Batch
gitpod /workspace/openlibrary (#8904-Second-Batch) $ 

@cdrini cdrini merged commit e52c680 into internetarchive:master Apr 24, 2024
3 checks passed
merwhite11 added a commit to merwhite11/openlibrary that referenced this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bulk internationalization of HTML title/values where needed
3 participants