Fix: Implement new layout in About page #27241
Conversation
|
@roryabraham I think the animation file needs to be corrected bcoz Screen.Recording.2023-09-12.at.17.13.18.mp4 |
|
Sorry for the delay @Pujan92, let me look into this |
|
@Pujan92 try this one please |
|
Regarding the safari thing – not sure. Can you try passing If that doesn't work, I think we'll need help 😅 |
|
Thanks @roryabraham. New File Screen.Recording.2023-09-14.at.11.05.19.movRenderer Canvas 
|
|
Or let me tag @shawnborton to check the current animation file once. Issue |
|
Hmm @roryabraham thoughts on this one? I thought we had this one working just fine? |
|
maybe I botched the JSON animation when I resized it? |
|
Finally managed to get this animation fixed, here you go: |
| <View style={[styles.settingsPageBody, styles.mb6, styles.alignItemsCenter, styles.ph5]}> | ||
| <Text style={[styles.baseFontStyle]}>{props.translate('initialSettingsPage.aboutPage.description')}</Text> | ||
| </View> |
There was a problem hiding this comment.
This is missing the "About Expensify" header
There was a problem hiding this comment.
@roryabraham could you plz provide a figma so I can verify "About Expnensify" header styles.
There was a problem hiding this comment.
Good spot @s77rt!
@Pujan92 I can't provide a Figma because I don't know where the mockup we need is in our Figma or how I could provide you that access. But I can tell you that the About Expensify text should be using styles.textHeadline. You'll have to make do with the screenshot to get the paddings / margins right.
Thanks @roryabraham, it looks fine now but still failing for iOS Safari. 😢 Screen.Recording.2023-09-20.at.16.14.06.mov |
Dang. Can we also just pass a smaller height on iOS Safari to shrink it back to the normal size? |
I mean it affects the height(188px) on all platforms 
|
|
What if we just do something like this? (haven't tested, may need some tweaks for the style and adding a constant for lottie renderer types, but you get the idea): diff --git a/src/components/IllustratedHeaderPageLayout.js b/src/components/IllustratedHeaderPageLayout.js
index 92a9c8b855..8bd31cc32a 100644
--- a/src/components/IllustratedHeaderPageLayout.js
+++ b/src/components/IllustratedHeaderPageLayout.js
@@ -31,15 +31,19 @@ const propTypes = {
/** Overlay content to display on top of animation */
overlayContent: PropTypes.func,
+
+ /** What renderer should we use for the Lottie animation? */
+ lottieRenderer: PropTypes.oneOf('svg', 'canvas'),
};
const defaultProps = {
backgroundColor: themeColors.appBG,
footer: null,
overlayContent: null,
+ lottieRenderer: 'svg',
};
-function IllustratedHeaderPageLayout({backgroundColor, children, illustration, footer, overlayContent, ...propsToPassToHeader}) {
+function IllustratedHeaderPageLayout({backgroundColor, children, illustration, footer, overlayContent, lottieRenderer, ...propsToPassToHeader}) {
const {isOffline} = useNetwork();
const {isSmallScreenWidth, windowHeight} = useWindowDimensions();
const appBGColor = StyleUtils.getBackgroundColorStyle(themeColors.appBG);
@@ -75,9 +79,10 @@ function IllustratedHeaderPageLayout({backgroundColor, children, illustration, f
<View style={[styles.alignItemsCenter, styles.justifyContentEnd, StyleUtils.getBackgroundColorStyle(backgroundColor)]}>
<Lottie
source={illustration}
- style={styles.w100}
+ style={lottieRenderer === 'svg' ? styles.w100 : styles.w70}
autoPlay
loop
+ renderer={lottieRenderer}
/>
{overlayContent && overlayContent()}
</View>
diff --git a/src/pages/settings/AboutPage/AboutPage.js b/src/pages/settings/AboutPage/AboutPage.js
index 0ef2a67f1a..9930ec9978 100644
--- a/src/pages/settings/AboutPage/AboutPage.js
+++ b/src/pages/settings/AboutPage/AboutPage.js
@@ -24,6 +24,7 @@ import IllustratedHeaderPageLayout from '../../../components/IllustratedHeaderPa
import themeColors from '../../../styles/themes/default';
import * as LottieAnimations from '../../../components/LottieAnimations';
import SCREENS from '../../../SCREENS';
+import * as Browser from '../../../libs/Browser';
const propTypes = {
...withLocalizePropTypes,
@@ -99,6 +100,7 @@ function AboutPage(props) {
illustration={LottieAnimations.Coin}
backgroundColor={themeColors.PAGE_BACKGROUND_COLORS[SCREENS.SETTINGS.ABOUT]}
overlayContent={overlayContent}
+ lottieRenderer={Browser.isSafari() ? 'canvas' : 'svg'}
>
<View style={[styles.settingsPageBody, styles.ph5]}>
<Text style={[styles.textHeadline, styles.mb1]}>{props.translate('footer.aboutExpensify')}</Text>
|
|
I still think height inconsistency(reason - reducing the width indirectly reduces the height for the correct aspect ratio) remains there compared to other animations for safari. 
Maybe you can checkout my branch to just give it a look in your local. |
|
Asked for help with to just get this animation working with the SVG renderer: https://expensify.slack.com/archives/C01GTK53T8Q/p1695655123405859 |
Yes @shawnborton , agree that looks big compare to others.
Yes plz. |
|
Sounds good, asking for a new file now, should have it in the next day or so. |
|
Updated version of the animation: Expensify-Loading-Coin-010924.zip |
Thanks @shawnborton.. WebScreen.Recording.2024-01-10.at.00.20.14.movIOSSimulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2024-01-10.at.00.22.47.mp4Androidab3.webm |
|
Lovely! |
|
@s77rt now it is ready for review! |
| const overlayContent = () => ( | ||
| <View style={[styles.pAbsolute, styles.w100, styles.h100, styles.justifyContentEnd, styles.pb5]}> | ||
| <Text | ||
| selectable | ||
| style={[styles.textLabel, styles.textIvoryLight, styles.alignSelfCenter]} |
There was a problem hiding this comment.
This is defining an anonymous component. On every render we define a new component and this may cause performance degradation as react will remove the child and create a completely new one. This is like changing the key prop every render. Let's use useCallback here so we keep the same component.
|
I noticed some delay in the Coin animation compared to others Screen.Recording.2024-01-09.at.11.37.29.PM.mov |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
Seems on first load sometimes it happens, I doubt it is bcoz of the file size(570KB) compare to others(30-40KB) |
|
Right that could be it. I don't think we should block the PR on this but it does seem like something we should improve |
| </Text> | ||
| </View> | ||
| ), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Add a comment explaining the reason
There was a problem hiding this comment.
I am not sure whether we require comment here bcoz isn't useCallback without dependencies descriptive enough to only create a function on the mount?
There was a problem hiding this comment.
I meant on why we are disabling the lint rule e.g. you can just write styles do not change
|
✋ 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/roryabraham in version: 1.4.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.25-10 🚀
|
Details
#26779
Fixed Issues
$ #26779
PROPOSAL: #26779 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2024-01-04.at.09.09.32.mov
Screen.Recording.2024-01-04.at.09.13.15.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android