Get rid of missing translation#42970
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
There is conflict |
|
@fedirjh thanks your review, i'm working on this issue now. |
|
@fedirjh I have finished updating according to your suggestions. |
fedirjh
left a comment
fedirjh
left a comment
There was a problem hiding this comment.
Let's remove function translateIfPhraseKey declaration as it's not used anymore.
These files needs to be updated :
src/pages/settings/Profile/Contacts/ValidateCodeForm/BaseValidateCodeForm.tsxsrc/pages/settings/Wallet/ExpensifyCardPage.tsx
src/pages/ReimbursementAccount/BusinessInfo/substeps/AddressBusiness.tsx
Outdated
Show resolved
Hide resolved
|
@nkdengineer There is a typescript check error and there is a failing unit test. |
|
Fixed now. |
|
@nkdengineer conflicts again |
|
For the second QA step:
Can we edit it to be a bit more specific? Specifically what form is it referring to (looks like it's the name step of the onboarding flow), and how do you force an error (e.g. use a reserved name like |
@francoisl Actually, we edited all forms in the codebase, not just forms but all components where we have been passing the translation keys instead of translated values. we just need to verify that the errors are displayed correctly. |
| @@ -1,6 +1,5 @@ | |||
| import type {AmountFormProps} from '@components/AmountForm'; | |||
| import type {MenuItemBaseProps} from '@components/MenuItem'; | |||
| import type {MaybePhraseKey} from '@libs/Localize'; | |||
There was a problem hiding this comment.
Looks like this type is not used anywhere anymore, we might as well remove it?
Yeah makes sense, maybe we can say something like "interact with forms in the app and fill them out incorrectly (e.g. missing values for required fields, values too long, etc.)". Although now that I think about it, most forms already have regression tests that would catch any issues so maybe we're fine. |
|
@francoisl @fedirjh i updated, please check again |
|
Aw there's a conflict in |
|
@francoisl fixed, please check again |
|
✋ 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/francoisl in version: 1.4.85-0 🚀
|
2 similar comments
|
🚀 Deployed to staging by https://github.com/francoisl in version: 1.4.85-0 🚀
|
|
🚀 Deployed to staging by https://github.com/francoisl in version: 1.4.85-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.85-7 🚀
|
| const {translate} = useLocalize(); | ||
| const input = useRef<BaseTextInputRef | null>(null); | ||
| const [login, setLogin] = useState(() => Str.removeSMSDomain(credentials?.login ?? '')); | ||
| const [formError, setFormError] = useState<TranslationPaths | undefined>(); |
There was a problem hiding this comment.
We had related issue when form error was shown and user changed language in login page the error would still be in old language. We basically applied old logic to this page reverting its changes. Unfortunately I didn't see this PR before our implementation, I would've discussed this more thoroughly. But so far our changes works as expected
Related issue: #44849
6 participants
Details
Fixed Issues
$ #42539
PROPOSAL: #42539 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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-resize.mp4
Android: mWeb Chrome
android-mweb-resize.mp4
iOS: Native
ios-resize.mp4
iOS: mWeb Safari
ios-mweb-resize.mp4
MacOS: Chrome / Safari
web-resize.mp4
MacOS: Desktop
desktop-resize.mp4