-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Clean up Localize.js #13778
Clean up Localize.js #13778
Conversation
@AndrewGable 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] |
Conflicts |
Updated! |
@AndrewGable I noticed you're super busy with expensiconx and margelo this week. I'm happy to reassign this to another reviewer if you'd like. |
Looks like some platforms are missing, can you update them? |
oh! strange. I thought I did those. Maybe I didn't save that edit 😅 |
Hey, I'm having a bit of an issue testing this on android chrome. Stuck on the splash screen 🤔 I need to run for tonight, but I'll come on back to this one tomorrow. Thanks @AndrewGable! |
@AndrewGable would you be willing to test this on my behalf while I try to get Android Chrome working? I've been trying different ways to get past the splash screen in the background today and I'm not getting any further with it. |
Going to try rebooting my vm and computer as a last ditch effort. |
OK I've got a lead on troubleshooting this here: https://expensify.slack.com/archives/C01GTK53T8Q/p1672959825844849?thread_ts=1672959207.942499&cid=C01GTK53T8Q |
So, I'm pretty much blocked on testing in Android Chrome right now. But all the other platforms show translations working, and my Android Chrome env isn't even working on the main branch, so I think this is a pretty safe PR to move forward with at this point. Ideally you'd be testing the 3 web platforms as a part of the review checklist anyway, so I think it'd be best to move forward with that and merge if you're feeling good. I know you're busy, so take your time, but please don't wait for me (this Android Chrome thing has been a persistent problem for me for the last few days, with no progress yet). |
I will help you diagnose in the thread! |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
Hey @AndrewGable, does the build from the comment above work for you on android chrome? If so, can we merge this? |
Hey @Luke9389 - Can we continue the conversation in the thread to track down your issues? If you cannot test on Android mWeb, you won't really be able to merge any PRs, so we should get that sorted as your first priority. |
True, and there isn't really a time constraint with this one. |
OK! Added the screenshot for Android Chrome from my physical device. Thanks for the help on this one @AndrewGable, I very much appreciate it. |
Reviewer Checklist
Screenshots/Videos |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
🚀 Deployed to staging by @AndrewGable in version: 1.2.53-0 🚀
|
🚀 Deployed to production by @Julesssss in version: 1.2.53-0 🚀
|
Details
I recently looked into our translation HOC, and noticed that the variable naming made the translate method super hard to read. This PR just cleans that up a little bit.
Fixed Issues
$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
QA Steps
Verify that no errors appear in the JS console
Go to Settings > Preferences and change the language to Spanish
Visit a bunch of different pages
Verify that none of the copy is broken.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android