-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Pass preferred locale to API on sign in #16326
Conversation
@0xmiroslav @Julesssss One of you needs to 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] |
@0xmiroslav actually that back-end PR has been merged but isn't on staging yet. I'll let you know when it is on staging so that you can test. |
Ok @0xmiroslav the back-end PR is on staging now, so you're free to test this. |
Sorry for the delay, been very busy these last few days. |
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.
Code looks good, and its testing well for me on web and Desktop.
Bump @0xmiroslav for the reviewer checklist. I'll be back tomorrow to re-test in any missing platforms.
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.
When Spanish user first install the app, Login page is in English which is default.
Should this be Spanish which depends on device locale?
@puneetlath please pull from latest |
@0xmiroslav done! |
Reviewer Checklist
Screenshots/VideosWebweb1.movweb2.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.mp4Desktopdesktop.moviOSios.mp4Androidandroid.mov |
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.
Great work! New Spanish users will love it! 😊
Tests well on all cases.
Created a follow up issue here in case you care to follow along: #16491 |
✋ 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/puneetlath in version: 1.2.90-0 🚀
|
@puneetlath it seems like this PR didn't pass QA. Would you mind taking a look? I won't block the deploy on this since it's a new feature and hasn't broken any functionality. |
@mvtglobally can you please share video of failing #15252? |
Ah crap. I didn't update the QA steps after updating the behavior in the PR. I updated the QA steps and confirmed it is working as expected on staging @luacmartins. |
@puneetlath please update QA Steps as well or just comment "Same as Tests step" |
Whoops. Both test steps and QA steps are updated now. |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.90-7 🚀
|
Details
This PR makes it so that we pass the preferred locale to the
SignInUser
API call if the user has set a locale other than the default. We only send it if it is something other than the default because the dropdown is pretty small and hard to find and setting it to something other than the default indicates they took an explicit action. The back-end changes to go along with this were done here.Fixed Issues
$ #15252
Tests
Offline tests
No offline tests. Signing in can only be done when online.
QA Steps
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.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android