Translation Migration common.iAcceptThe split phrase using RenderHTML#73191
Conversation
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report❌ Patch coverage is
... and 12 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Reviewer Checklist
Screenshots/Videos |
|
@PiyushChandra17, looks like you have only provided steps for one page and added screenshots for that. Could you add the steps to test other pages and screenshots? Also please add screenshots for native. |
src/languages/en.ts
Outdated
| iAcceptTheFull: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a> and <a href="${CONST.OLD_DOT_PUBLIC_URLS.PRIVACY_URL}">Privacy Policy</a>`, | ||
| iAcceptTheConfirmAgreements: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.ACH_TERMS_URL}">terms and conditions</a>`, | ||
| iAcceptTheConfirmation: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a>`, |
There was a problem hiding this comment.
@aldo-expensify, any reason for keeping 2 different translations? We use I accept the Expensify Terms of Service and Privacy Policy at payments page and I accept the Expensify Terms of Service at wallet substep. Can we use the full version for both?
There was a problem hiding this comment.
Well, the terms are different in each case (different URL), so I think it is fine that the text is also different. One is our terms, the other seems to be ACH terms.
There was a problem hiding this comment.
@aldo-expensify, the translations I mentioned have same url (1st and 3rd) for "Expensify Terms of Service", the translation only have an extra part for privacy policy in payments page. I think we should confirm with the team.
There was a problem hiding this comment.
I don't think changing this in the scope of this PR, and I'm not sure why would want to start asking the user to read and confirm the Privacy policy if that was not the case before.
There was a problem hiding this comment.
I'm not sure why would want to start asking the user to read and confirm the Privacy policy if that was not the case before.
I think it's possible that it was missed as there was a similar case in past. But as it's not in the scope of this PR we can skip the discussion here. Will confirm it with design team on slack later.
@thesahindia Sure, will update the steps and screenshots. Thanks for the review. |
src/languages/en.ts
Outdated
| iAcceptTheFull: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a> and <a href="${CONST.OLD_DOT_PUBLIC_URLS.PRIVACY_URL}">Privacy Policy</a>`, | ||
| iAcceptTheConfirmAgreements: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.ACH_TERMS_URL}">terms and conditions</a>`, | ||
| iAcceptTheConfirmation: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a>`, |
There was a problem hiding this comment.
| iAcceptTheFull: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a> and <a href="${CONST.OLD_DOT_PUBLIC_URLS.PRIVACY_URL}">Privacy Policy</a>`, | |
| iAcceptTheConfirmAgreements: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.ACH_TERMS_URL}">terms and conditions</a>`, | |
| iAcceptTheConfirmation: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a>`, | |
| acceptTermsAndPrivacy: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a> and <a href="${CONST.OLD_DOT_PUBLIC_URLS.PRIVACY_URL}">Privacy Policy</a>`, | |
| acceptTermsAndConditions: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.ACH_TERMS_URL}">terms and conditions</a>`, | |
| acceptTermsOfService: `I accept the <a href="${CONST.OLD_DOT_PUBLIC_URLS.TERMS_URL}">Expensify Terms of Service</a>`, |
There was a problem hiding this comment.
@aldo-expensify Do we really need to change the translation keys here, iAcceptTheFull, iAcceptTheConfirmAgreements, and iAcceptTheConfirmation. I think this makes more sense to me w.r.t file names ...
There was a problem hiding this comment.
For me any of the options is fine, so I would consider it a NAB 🤷
There was a problem hiding this comment.
@thesahindia Your thoughts on this? As it's not a blocker,I think we should move ahead with this
There was a problem hiding this comment.
I think the suggested ones will be easier to read and understand compared to keys like "iAcceptTheFull." It’s a simple change, update it if you’d like, as Aldo mentioned, it’s a NAB.
|
Hey, I noticed you changed Please look at the code and make sure there are no malicious changes before running the workflow. If you have the K2 extension, you can simply click: [this button] |
@thesahindia Yes i am looking into this, i think this is quite similar and i am quite sure i have implemented correctly, just to double check will update that , perhaps i think that needs some workaround to repro the steps. However if you can repro that steps that would be helpful (where it exists in the UI)
I think my Note: All checks have passed ✅ |
|
@thesahindia Updated the "acceptTermsOfService" repro steps and screenshots. Note: "acceptTermsAndConditions" would be damn similar indeed. Thanks |
|
@thesahindia Bump |
|
@thesahindia Bump |
|
@thesahindia Update the "acceptTermsAndConditions" repro steps (needed some workaround here) and screenshots. Note: For the first part, we can't make changes to "Subscription" in Native, thats the reason screenshots aren't there. cc: @aldo-expensify |
|
@aldo-expensify Bump |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.2.43-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.43-2 🚀
|






Explanation of Change
Translation Migration common.iAcceptThe split phrase using
RenderHTMLFixed Issues
$ #72049
PROPOSAL: #72049 (Comment)
Tests
Part 1
Part 2
Part 3
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
"Same as tests"
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop