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(ci): disable placeholder conversion during i18n-update-push #656

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

zyf722
Copy link
Contributor

@zyf722 zyf722 commented Nov 27, 2024

What type of PR is this? (check all applicable)

  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • ♻️ Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert
  • 🌐 Internationalization / Translation

Description

By default, Lokalise's file upload API will try to convert placeholders into their universal format, which is not our desired behavior and causes several QA issues ("Placeholder/HTML mismatch") to be raised.

This PR fixes this issue by explicitly setting the convert_placeholders flag to false.

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentations?

  • πŸ““ docs (./docs)
  • πŸ“• storybook (./storybook)
  • πŸ“œ README.md
  • πŸ™… no documentation needed

Are there any post-deployment tasks we need to perform?

After merge, please manually run the following to fix data existed on the branch gigamaster/dev-gui which will be used for next release:

npm run i18n-export
npm run i18n-update-push -- gigamaster/dev-gui --force

Note that LOKALISE_PROJECT_ID and LOKALISE_API_TOKEN should be set before running these commands.

I've already done a test run with my fork repo and the staging project on Lokalise. You might want to have a test there as well - just fill LOKALISE_PROJECT_ID with 60810062671f99b3883fb4.07995509, replace gigamaster/dev-gui with master and everything should be fine.

Whether the data on the master branch should be updated might be left for discussion. We can either update it using i18n entries exported from the codebasse before merging #630, or just leave it as is for now and update it later when merging gigamaster/dev-gui to master on Lokalise.

Copy link

netlify bot commented Nov 27, 2024

βœ… Deploy Preview for livecodes ready!

Name Link
πŸ”¨ Latest commit b13e290
πŸ” Latest deploy log https://app.netlify.com/sites/livecodes/deploys/67476af255155400081ab357
😎 Deploy Preview https://deploy-preview-656--livecodes.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

sonarcloud bot commented Nov 27, 2024

@hatemhosny hatemhosny merged commit 04867a1 into live-codes:develop Nov 27, 2024
11 checks passed
@hatemhosny
Copy link
Collaborator

Thank you @zyf722

After merge, please manually run the following to fix data existed on the branch gigamaster/dev-gui which will be used for next release

Done.
Would you please check?

Whether the data on the master branch should be updated might be left for discussion. We can either update it using i18n entries exported from the codebasse before merging #630, or just leave it as is for now and update it later when merging gigamaster/dev-gui to master on Lokalise.

I think leave it till we merge gigamaster/dev-gui

@hatemhosny
Copy link
Collaborator

I see the inconsistent placeholders here:
https://app.lokalise.com/project/34958094667a72e9454592.95108106/?view=multi&filter=builtin_21&branch=gigamaster/dev-gui

I added translations for this key on Lokalise UI when we used #640 as a test.

@zyf722
Copy link
Contributor Author

zyf722 commented Nov 28, 2024

I see the inconsistent placeholders here:
app.lokalise.com/project/34958094667a72e9454592.95108106?view=multi&filter=builtin_21&branch=gigamaster/dev-gui

The error in this entry only exists in translations except English, and it probably comes from a previous manual upload with Convert to universal placeholders set to true. I've just fixed it manually, and there are no more entries with this error now.

By the way, I would suggest we address the QA issues reported as of now before refining other entries. You might also want to take a look at the QA checks in the project settings to configure check QA gates.

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.

2 participants