-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Create status loading skeleton #47515
Conversation
@rayane-djouah 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] |
@rayane-djouah 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] |
The skeleton looks good to me, but I think we should try and limit the use of it by only doing the skeleton loader on initial load of the page. When swapping between the tabs themselves, I don't think we should swap over to skeletons again cause it leads to a lot of "flickering" or jarriness (that's a word right!?). Or perhaps this is just how you're showing it for pure debug purposes? Keen to hear rest of @Expensify/design 's thoughts on this too |
Totally agree with Jon here. Let's only use it when you first load one of the expense types from the LHN, otherwise navigating between each tab doesn't need the tab skeletons. |
Big agree with the other designers. The only time I think we may want to switch back to the skeleton for the status is when you're changing the selected LHN item/type (i.e. Expenses, Invoices, Trips). Because the statuses are linked to the type, they may be different across types (especially as we add more). So I think it might make sense to use the skeleton in that situation. |
The holding PR is merged |
On hold for #47753 |
@luacmartins should we address #47515 (comment) in this PR or in a follow-up PR? |
I'll come back to this PR since we merged the underlying dependency. I'll update it soon. |
@rayane-djouah updated! Screen.Recording.2024-08-28.at.5.40.26.PM.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-08-29.at.12.15.21.PM.movAndroid: mWeb ChromeScreen.Recording.2024-08-29.at.12.11.27.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-29.at.12.11.04.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-29.at.12.06.22.mp4MacOS: Chrome / SafariScreen.Recording.2024-08-29.at.12.19.16.PM.movMacOS: DesktopScreen.Recording.2024-08-29.at.12.10.05.PM.mov |
Bug? When we navigate to Filters > View Results, the status loading skeleton is displayed even though the type has not changed. Screen.Recording.2024-08-29.at.12.20.45.PM.mov |
@rayane-djouah After I merged main, we no longer keep the Screen.Recording.2024-08-29.at.12.05.23.PM.mov |
@luacmartins I think what you're showing in that video looks correct to me. |
Nice! Let's continue with review then 😄 |
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.
LGTM and tests well
@cristipaval 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] |
Checklist is already completed. @cristipaval all yours for the final review! |
✋ 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/cristipaval in version: 9.0.27-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.27-1 🚀
|
Details
Adds a loading skeleton to the Search status bar
Fixed Issues
$ #46582
Tests
Offline tests
N/A
QA Steps
Same as tests
PR 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./** 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
)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web-l.mov
web-n.mov
MacOS: Desktop