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

1425: Import Spanish Translations #288

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

GeorgeCodes19
Copy link
Contributor

@GeorgeCodes19 GeorgeCodes19 commented Sep 24, 2024

Ticket

Resolves FFS-1425

Changes

Added a TranslationService that I will attempt to make as generic as possible in order to import various other translations

  • TranslationService includes the ability to register multiple modifiers in order to update an individual row's values
  • TranslationService includes the ability to skip rows by registering a method to skip rows.

I believe that the two middlewares above can be joined into a single function that operates on a per row basis. Returning false could skip the row while other operations could allow the row value to be modified in the lambda/closure.

Context for reviewers

We'll still want to walk through each page and translation.

Testing

Tested using the Rails console, rspec, and visually.

Screenshot 2024-09-23 at 8 26 05 PM

Screenshot 2024-09-23 at 8 20 14 PM
Screenshot 2024-09-23 at 8 20 53 PM
Screenshot 2024-09-23 at 8 19 53 PM

Copy link

@carmenrosalop carmenrosalop left a comment

Choose a reason for hiding this comment

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

Thanks @GeorgeCodes19 I'm so glad the translations process was smooth. The CBV pages look great! I'd like to see how the client-facing PDF report in Spanish looks like. Could you please add it here?

@GeorgeCodes19
Copy link
Contributor Author

Thanks @GeorgeCodes19 I'm so glad the translations process was smooth. The CBV pages look great! I'd like to see how the client-facing PDF report in Spanish looks like. Could you please add it here?

Yes! I'll actually send you the PDFs docs and attach images.

@GeorgeCodes19 GeorgeCodes19 marked this pull request as ready for review September 25, 2024 18:10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we delete this or have a version that's scrubbed a bit more before adding it to the repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't think it's very useful to commit to the repo. If you want to commit something so you can test your script, can you just commit the first couple lines?

Copy link
Contributor

@tdooner tdooner left a comment

Choose a reason for hiding this comment

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

I didn't really review the script, because if it worked to import the spanish CSV into the YAML here, that's good enough for me. Some documentation might be useful as a comment in that file that describes how to call the code.

@GeorgeCodes19 GeorgeCodes19 merged commit 792cf5b into main Sep 26, 2024
12 checks passed
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.

3 participants