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(tests, charlie): Add menu translation keys #6134

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

BenJamesBen
Copy link
Contributor

@BenJamesBen BenJamesBen commented Feb 23, 2024

This PR includes a change to the design i18n tests to allow design menu translation keys. If o.<key>.d is set to menu, it means that the item is menu name and not an actual option. This causes the test to check for an actual option to be skipped.

So, to add a translation key for a menu:

   "o": {
    "myMenu": {
      "t": "My Menu",
      "d": "menu"
    },

This PR also adds 3 menu translations for Charlie.

Copy link

vercel bot commented Feb 23, 2024

Someone is attempting to deploy a commit to the freesewing Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Feb 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
freesewing-dev ⬜️ Ignored (Inspect) Visit Preview Feb 23, 2024 4:33am

@boring-cyborg boring-cyborg bot added the 🧪 tests Tests make sure things are ok label Feb 23, 2024
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.21%. Comparing base (13f492a) to head (ff5b124).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #6134   +/-   ##
========================================
  Coverage    97.21%   97.21%           
========================================
  Files           15       15           
  Lines         4493     4493           
  Branches       535      535           
========================================
  Hits          4368     4368           
  Misses         122      122           
  Partials         3        3           
Flag Coverage Δ
core 97.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@BenJamesBen BenJamesBen changed the title fix(charlie): Add menu translation keys fix(tests, charlie): Add menu translation keys Feb 23, 2024
@BenJamesBen
Copy link
Contributor Author

BenJamesBen commented Feb 23, 2024

I'm thinking that using the string menu might cause confusion in Crowdin, if the translators mistakenly think that the word should be translated. it might avoid confusion if we were to use a different string like M or M1 or MMM. Something that the translators will just leave as-is.

Or, perhaps there might be a better way to provide menu translations for designs?

@BenJamesBen BenJamesBen marked this pull request as draft February 23, 2024 15:03
@BenJamesBen
Copy link
Contributor Author

Reworking this PR to not use the "o" section in the i18n json files.

@joostdecock
Copy link
Member

Reworking this PR to not use the "o" section in the i18n json files.

Didn't see this comment earlier but I appreciate the effort

@BenJamesBen
Copy link
Contributor Author

I am still working on this PR, though I've been sidetracked the past few weeks.
I think that the idea is to add a new m top-level property to the JSON files for menu translations, with t and d sub properties (since the UI code already works with .t text keys). This will require changing docs, utils.mjs, and prebuild scripts, and adding tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👕 charlie 🧪 tests Tests make sure things are ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants