Update Preferences page with push-to-page design#14591
Conversation
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-02-04.at.12.13.22.AM.movMobile Web - ChromeScreen.Recording.2023-02-04.at.12.21.53.AM.movMobile Web - SafariScreen.Recording.2023-02-04.at.12.16.23.AM.movDesktopScreen.Recording.2023-02-04.at.12.23.14.AM.moviOSScreen.Recording.2023-02-04.at.12.20.57.AM.movAndroidWhatsApp.Video.2023-01-31.at.19.29.44.1.mp4 |
Beamanator
left a comment
There was a problem hiding this comment.
Thanks for all the changes @cristipaval ! Just a few tiny ones left!
|
All feedback is addressed. Another round of review please? |
rushatgabhane
left a comment
There was a problem hiding this comment.
Tests well! Another small comment
|
|
||
| return ( | ||
| <ScreenWrapper includeSafeAreaPaddingBottom={false}> | ||
| <HeaderWithCloseButton |
There was a problem hiding this comment.
@cristipaval we should put all this inside ScrollView, right?
To make it scrollable when new items are adding to this page.
There was a problem hiding this comment.
Should we? Isn't OptionList handling the scroll if needed?
There was a problem hiding this comment.
@cristipaval i was thinking we'll need ScrollView when items other than OptionList are added. (Images, Text etc).
There was a problem hiding this comment.
We definitely might need ScrollView eventually, but we can add that later when / if we add those other components
|
|
||
| return ( | ||
| <ScreenWrapper includeSafeAreaPaddingBottom={false}> | ||
| <HeaderWithCloseButton |
There was a problem hiding this comment.
Yeah agreed I don't think scrollview is needed here yet
rushatgabhane
left a comment
There was a problem hiding this comment.
question: should we add arrow key navigation support?
There was a problem hiding this comment.
Only 1 change I'd "Like" to see (about navigating in the action file) but I don't think it's big enough to block (you could take care of this in your follow-up PR)
Nice work on all of this by the way @cristipaval 💪
| if (language.value !== props.preferredLocale) { | ||
| App.setLocale(language.value); | ||
| } | ||
| Navigation.navigate(ROUTES.SETTINGS_PREFERENCES); |
There was a problem hiding this comment.
@cristipaval I know you want to get this out soon, we really should move this to an action, maybe called setLocaleAndNavigate (since we have functions with similar names). Then that function can call App.setLocale and then navigate. I think we really try to avoid inlining functions like this, too
|
|
||
| {/* If we are in the staging environment then we enable additional test features */} | ||
| { | ||
| _.contains([CONST.ENVIRONMENT.STAGING, CONST.ENVIRONMENT.DEV], props.environment) |
There was a problem hiding this comment.
NAB since this line is kinda long and makes the conditional render a bit ugly, we "could" store this in a variable (like shouldShowTestToolMenu) and then just do something like:
{shouldShowTestToolMenu && (
<...jsx stuff>
)}|
|
||
| return ( | ||
| <ScreenWrapper includeSafeAreaPaddingBottom={false}> | ||
| <HeaderWithCloseButton |
There was a problem hiding this comment.
Yeah agreed I don't think scrollview is needed here yet
|
I say let's merge since it will be nice to have this refreshed page UI, @cristipaval do you mind addressing my previous tiny comments in your follow-up PR? 🙏 |
|
✋ 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 https://github.com/Beamanator in version: 1.2.66-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.2.66-0 🚀
|
Details
Aligns Preferences page with the push-to-page design cc @Beamanator
Fixed Issues
$ #14432
Tests and QA Steps
Settings -> PreferencesOffline tests
https://user-images.githubusercontent.com/18078393/215102142-5c57a469-4336-45db-b2e9-fe909ed77293.mov
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
android.chrome.mov
Mobile Web - Safari
ios.safari.mov
Desktop
desktop.mov
iOS
ios.native.mov
Android
android.native.mov