Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(suite-native): show fiat balance skeleton while graph is loading #16217

Merged
merged 1 commit into from
Jan 9, 2025
Merged

fix(suite-native): show fiat balance skeleton while graph is loading #16217

merged 1 commit into from
Jan 9, 2025

Conversation

yanascz
Copy link
Contributor

@yanascz yanascz commented Jan 7, 2025 •
edited
Loading

The missing skeleton was causing unpleasant layout shifts.

Screen.Recording.2025-01-09.at.13.38.46.mov

@yanascz yanascz requested a review from a team as a code owner January 7, 2025 10:08
@yanascz yanascz self-assigned this Jan 7, 2025
@yanascz yanascz added the mobile Suite Lite issues and PRs label Jan 7, 2025
@yanascz yanascz force-pushed the fix/native/graph-loading-state branch from 9a76a08 to be2f6d5 Compare January 7, 2025 11:28
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025 •
edited
Loading

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 21
  • More info

Learn more about 𝝠 Expo Github Action

@Nodonisko
Copy link
Contributor

Does this show skeleton also when changing graph timeframe? Probably fine but just aksing

@vytick
Copy link
Contributor

vytick commented Jan 8, 2025

Tests are failing

@PeKne
Copy link
Contributor

PeKne commented Jan 9, 2025

I reran the E2E test and it passed this time. But I've prepared E2E fix anyway. So if you experience any flakiness again, please make sure that you are above commit of this PR: #16276

@yanascz
Copy link
Contributor Author

yanascz commented Jan 9, 2025 •
edited
Loading

Does this show skeleton also when changing graph timeframe? Probably fine but just aksing

Nope, see the newly attached video in issue description.

@yanascz yanascz force-pushed the fix/native/graph-loading-state branch from be2f6d5 to 22a087b Compare January 9, 2025 12:41
matejkriz
matejkriz reviewed Jan 9, 2025
View reviewed changes
suite-native/module-home/src/screens/HomeScreen/components/PortfolioContent.tsx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not related to fiat balance skeleton, right? Or is it somehow needed because of the other changes in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's related to PortfolioHeader in a way and was discovered during testing. @shenkys wanted the button to appear once discovery finishes.

We may do that in a separate PR if you insist.

matejkriz
matejkriz approved these changes Jan 9, 2025
View reviewed changes
Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for device authorization for Receive button seems unrelated to this PR, but otherwise it looks good to me and I see no jumps while testing.

@yanascz yanascz merged commit 132e7c6 into develop Jan 9, 2025
16 checks passed
@yanascz yanascz deleted the fix/native/graph-loading-state branch January 9, 2025 16:28
@STew790
Copy link
Contributor

STew790 commented Jan 15, 2025

QA OK
LGTM and feels smooth.

Screenrecorder-2025-01-15-16-47-40-138.mp4

Info
25.1.2 995410a

@STew790 STew790 added the QA OK Issue passed QA without any blocker label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matejkriz matejkriz matejkriz approved these changes

Assignees

@yanascz yanascz

Labels
mobile Suite Lite issues and PRs QA OK Issue passed QA without any blocker
Projects
Suite Mobile
Status: ✅ Approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yanascz @Nodonisko @vytick @PeKne @STew790 @matejkriz