Empty State Component for Search#44137
Conversation
kosmydel
left a comment
There was a problem hiding this comment.
LGTM, I've left some minor comments 🚀
| import CONST from '@src/CONST'; | ||
| import type {EmptyStateComponentProps, VideoLoadedEventType} from './types'; | ||
|
|
||
| const VIDEO_ASPECT_RATIO = 400 / 225; |
There was a problem hiding this comment.
I'm not sure, but shouldn't it be included in CONSTs?
Will the aspect ratio always be the same?
There was a problem hiding this comment.
It may be different for each usage. Here it is adjusted to the "Modal" size
There was a problem hiding this comment.
It would be great to leave a comment about this, how did we get to this ratio?
|
@shawnborton @eVoloshchak 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] |
This comment has been minimized.
This comment has been minimized.
|
Lovely! Okay, l think we should get this thing into final review then? |
|
Awesome, I see @eVoloshchak didn't finish the checklist |
mountiny
left a comment
There was a problem hiding this comment.
Thank you! Small comments
| import CONST from '@src/CONST'; | ||
| import type {EmptyStateComponentProps, VideoLoadedEventType} from './types'; | ||
|
|
||
| const VIDEO_ASPECT_RATIO = 400 / 225; |
There was a problem hiding this comment.
It would be great to leave a comment about this, how did we get to this ratio?
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
So this is looking awesome @filip-solecki but I found one little bug here - it seems like the layout slightly shifts when the empty state loads right after the loading skeleton: CleanShot.2024-07-15.at.22.30.39.mp4Seems like we might just need some more horizontal padding around the loading skeleton? The whole view should be wrapped with 20px horizontal padding, but it looks like there is a difference between the two. Do you mind creating a follow up PR when you get a moment? Thanks! |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.7-3 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/thienlnam in version: 9.0.7-4 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
Sure, I'll prepare follow-up PR probably today |
|
Perfect, thanks Filip! |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.7-8 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.8-6 🚀
|
|
Friendly bump on those follow up items @filip-solecki |
|
Only this comment left right? |
|
I think so. We implemented this for report fields, right? |
|
Yes, here |
|
Cool, thanks for confirming! |
|
|
||
| return ( | ||
| <ScrollView contentContainerStyle={styles.emptyStateScrollView}> | ||
| <View style={styles.skeletonBackground}> |
There was a problem hiding this comment.
Having this view inside the scrollview was causing the skeleton to become scrollable at a certain page height, we fixed this in #57232





Details
We want to make empty states feel nicer by using a subtle hint that the page will eventually contain data by using a faded row pattern, while also creating space for the middle of the page to show an informative or educational modal.
Fixed Issues
$ #43747
PROPOSAL: N/A
Tests
Searchpage fromLHPWorkspacesTagsand openTagspageCategoriespage and remove all categoriesOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop