-
Notifications
You must be signed in to change notification settings - Fork 38
Update PT Translations and usage #2404
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
base: feature/check-memo-required-on-send
Are you sure you want to change the base?
Update PT Translations and usage #2404
Conversation
…d-pt-missing-translations
…lar/freighter into feature/add-pt-missing-translations
| page.getByTestId("asset-on-list").waitFor({ state: "visible" }), | ||
| ]); | ||
|
|
||
| if (await notOnLists.isVisible()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we removing these tests?
| await page.locator("#termsOfUse-input").check({ force: true }); | ||
| await page.getByText("Confirm").click(); | ||
|
|
||
| // GDF32CQINROD3E2LMCGZUDVMWTXCJFR5SBYVRJ7WAAIAS3P7DCVWZEFY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is useful to let devs know the mnemonic phrase below maps to this G address - we check the UI for this specific G address a few times in these tests
| await page.getByTestId("display-mnemonic-phrase-confirm-btn").click(); | ||
| await expect(page.getByText("You’re all set!")).toBeVisible(); | ||
|
|
||
| // Wait for navigation to success page (create wallet uses different route than import) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a lot of seemingly unrelated changes to tests in this PR. Was this the only way you could get your tests to run? It'd be helpful from a reviewer perspective to understand why we need all these changes
| ); | ||
| expect(screen.getByTestId("tile-secondary")).toHaveTextContent( | ||
| "Choose asset", | ||
| "Choose Asset", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the motivation to switch casing for all these labels, but now this doesn't match our figma. We should be chatting with @sdfcharles before making sweeping changes like this. He may be fine with this, but we should be having a conversation about it
|
|
||
| // Custom transform function that preserves existing translations | ||
| // Only adds new keys, never overwrites existing values | ||
| function preserveExistingTranslations(locale, namespace, key, value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want this? This seems like a significant custom implementation and it's not clear to why we need something that I18nextWebpackPlugin doesn't provide out of the box
| ? currentTransactionFee | ||
| : simParams.type === "classic" | ||
| ? simParams.transactionFee | ||
| : transactionFee; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these fee changes be part of this translations PR?
Closes #2401
Screen.Recording.2025-11-24.at.15.50.11.mov