-
Notifications
You must be signed in to change notification settings - Fork 33
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
PM-16847: Update inline loading indicators #1258
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1258 +/- ##
=======================================
Coverage 89.42% 89.43%
=======================================
Files 725 725
Lines 45808 45824 +16
=======================================
+ Hits 40966 40983 +17
+ Misses 4842 4841 -1 ☔ View full report in Codecov by Sentry. |
Great job, no security vulnerabilities found in this Pull Request |
.onAppear { | ||
isSpinning = true | ||
withAnimation(.linear(duration: 1).repeatForever(autoreverses: false)) { | ||
isSpinning = true |
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.
🤔 For my own edification, what's the salient difference between these two?
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.
With the previous implicit animation, when the vault list would load I think the search bar ends up moving the spinner as it appears and that was getting picked up in the animation. So the spinner would not only rotate, but it would also move vertically 🙃. Switching to using withAnimation
fixed that.
loading.animation.mp4
let needsSync = try await services.vaultRepository.needsSync() | ||
|
||
if needsSync, sections.isEmpty { | ||
// If a sync is needed and there's no sends in the user's vault, it could |
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.
⛏️ Technically, it would be "there are no sends"
🎟️ Tracking
PM-16847 - Partial Loading Indicator: My Vault
PM-16848 - Partial loading indicator: View item
PM-16849 - Partial loading indicator: Sends
📔 Objective
Updates the inline loading indicator for the following screens:
The send list was missing a loading indicator, so that was added as well.
📸 Screenshots
loading.mp4
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes