-
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
fix: Update Wallet and Generic Folder empty state to use lottie animations. #48040
fix: Update Wallet and Generic Folder empty state to use lottie animations. #48040
Conversation
…tions. Signed-off-by: krishna2323 <[email protected]>
@hungvu193 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] |
Signed-off-by: krishna2323 <[email protected]>
Can you show me an example of that? I don't feel too strongly here... the 32px keeps things fluffy but I can see where it might feel off given that the workspace cards and other places go from 32px to 20px. |
@Krishna2323 everything is looking good to me, including button placement for categories! I'm going to follow up and get you a smaller version of the folder animation so that it feels more close to the older static version we had. |
I'd love to see the |
Updated Lottie file is here: GenericEmptystate-082724-1.json.zip @roryabraham can you compress that for us when you get a chance? Thanks! |
unfortunately LottieFiles seems to be down this morning and it's not letting me upload anything. I'll have to try again later. I'll set a reminder |
Sounds good, keep us posted! |
@Krishna2323 would it be easy for you to change the padding of this area on mobile: |
Yes, will do that. |
Updated animation GenericEmptyState animation: GenericEmptyState.lottie.zip |
|
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
@Expensify/design, I have updated the empty state animation, and it looks correct to me, but I noticed one weird behaviour on the report field list page. When the RHP opens, the animation in the background stops. I’ve been trying to find a solution for that, but I haven't found any clues yet. @hungvu193, do you have any ideas about this? Monosnap.screencast.2024-08-29.06-48-41.mp4 |
It came from #47960, we're hiding lottie view when navigation is not focused. Screen.Recording.2024-08-29.at.14.11.36.movApp/src/components/Lottie/index.tsx Lines 35 to 37 in b9af3d1
App/src/components/Lottie/index.tsx Lines 47 to 49 in b9af3d1
|
It was aimed to fix #47273. So I think that wont be an issue then? Would love to hear from design team. |
@dubielzyk-expensify why do you think it looks wrong? I guess it's technically an empty list, so we're showing the empty state we use for empty lists there... I don't feel strongly though, I could see using a lighter empty state for this one. |
I think ideally the animation would still play in the background when the RHP is open though... so I would love to solve that if possible. |
Can you please comment in the original issue that I mentioned above?. It's not related to this PR 😄 |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
@shawnborton @dannymcclain, looks better now? Monosnap.screencast.2024-09-18.18-22-54.mp4 |
I think that's looking pretty good to me! Will wait for Shawn's feedback too. |
Signed-off-by: krishna2323 <[email protected]>
That does feel better, yup. |
Gonna run a test build but let's get this into final review! |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Hmm I'm definitely not trying to be difficult, but in this case here I would still expect the animation to be 220px tall - it would just be horizontally centered within the available background area. Instead it looks like we are shrinking based on the 400px width that it has: Perhaps it's a simple fix? Take a look at this: CleanShot.2024-09-18.at.23.59.26.mp4 |
Yeah, seems like a easy fix. Let me check. |
Signed-off-by: krishna2323 <[email protected]>
@@ -5168,10 +5170,11 @@ const styles = (theme: ThemeColors) => | |||
|
|||
emptyStateFolderWebStyles: { | |||
...sizing.w100, | |||
minWidth: 300, | |||
minWidth: 400, |
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.
@Krishna2323 Friendly reminder that you need to make sure you test this one on smaller device like iPhone8 or iPhoneSE
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.
Yeah, thanks for the reminder. I tested on the web by resizing, but I still need to test on all 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.
@hungvu193, tested on all platforms. I think you start reviewing now.
Monosnap.screencast.2024-09-20.04-45-12.mp4
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.
Thanks man! Appreciate it, I'll give it a final review today!
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Screen.Recording.2024-09-20.at.10.54.09.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-09-20.at.13.53.41.movAndroid: mWeb ChromeScreen.Recording.2024-09-10.at.17.46.15.moviOS: NativeScreen.Recording.2024-09-10.at.16.12.34.moviOS: mWeb SafariScreen.Recording.2024-09-10.at.15.50.42.movMacOS: Chrome / SafariScreen.Recording.2024-09-10.at.15.21.33.movMacOS: DesktopScreen.Recording.2024-09-10.at.15.14.28.mov |
minModalHeight = 400, | ||
}: EmptyStateComponentProps) { | ||
const styles = useThemeStyles(); | ||
const [videoAspectRatio, setVideoAspectRatio] = useState(VIDEO_ASPECT_RATIO); | ||
const {isSmallScreenWidth} = useResponsiveLayout(); |
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.
NAB because it seems like all the styles are right in the screen recordings, but I think we might be able to use shouldUseNarrowLayout
here, and generally that should be the default go-to instead of isSmallScreenWidth
✋ 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: 9.0.40-0 🚀
|
This PR seem to have caused a regression #49744 |
This PR only updated the lottie animation files so it won't cause that regression. Ideally, the regression will be from one of the PR that updated LottieView recently. |
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.40-6 🚀
|
Details
Fixed Issues
$ #48013
PROPOSAL: #48013 (comment)
Tests
Trips
from LHS (filters) > Verify Trips Empty State Animation is shownBook travel
> Verify Trips Empty State Animation is shownAdd field
> Select list as typesettings/wallet
> Verify Bank Vault animation is shownOffline tests
Trips
from LHS (filters) > Verify Trips Empty State Animation is shownBook travel
> Verify Trips Empty State Animation is shownAdd field
> Select list as typesettings/wallet
> Verify Bank Vault animation is shownQA Steps
Trips
from LHS (filters) > Verify Trips Empty State Animation is shownBook travel
> Verify Trips Empty State Animation is shownAdd field
> Select list as typesettings/wallet
> Verify Bank Vault animation is shownPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4