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.

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 initial load issue #1284

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Apr 8, 2024

Pull Request Description

This PR fixes an issue where there's not an easy way to retry loading the feed if it times out due to a network issue. Note that in addition to changing the ErrorMessage action from "Account Settings" to "Retry", I also made the action button disabled and show a spinner when it is pressed. I did this because the main page doesn't react to AuthStatus.loading, so there was no other indication that any action had been taken. I'm not sure exactly why that was, but I have a feeling it had something to do with the main page indicator and the feed page's indicator not quite aligning together, and most of the time only the latter is needed.

Review without whitespace.

Issue Being Fixed

Issue Number: #1283

Screenshots / Recordings

The demo shows first an unsuccessful retry attempt, and then a successful one.

qemu-system-x86_64_zTWK31Qo21.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu
Copy link
Member

hjiangsu commented Apr 10, 2024

changing the ErrorMessage action from "Account Settings" to "Retry"

I think this causes a regression where we can't switch to a different account if the current account's instance is down. See #769 and #770 for more details.

To prevent this, I think we can introduce two actions here: one to retry and one to switch accounts. Thoughts?

@micahmo
Copy link
Member Author

micahmo commented Apr 10, 2024

Good call! I've made ErrorMessage more generic so it can take a list of actions. Let me know what you think!

qemu-system-x86_64_YoWBgRLugG.mp4

@hjiangsu
Copy link
Member

I just have one small UI tweak - can we make it so that the buttons are in separate rows? Also, I think making one button the "primary" action and the other a "secondary" action would help with overall UI/UX!

For the primary/secondary, I'd suggest the following:

  • Primary button uses the current look (elevated) - this can be the "Retry" option
  • Secondary button uses a TextButton - this one can be the "Account Settings" option

@micahmo
Copy link
Member Author

micahmo commented Apr 14, 2024

can we make it so that the buttons are in separate rows?

I actually tried this first and liked side-by-side better, but I'll put it back!

Secondary button uses a TextButton - this one can be the "Account Settings" option

Alright, I went ahead and pushed this. It looks like below. Is that ok?

image

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

LGTM! Just double checking, this is ready to be merged?

@@ -4,10 +4,14 @@ import 'package:flutter_gen/gen_l10n/app_localizations.dart';
class ErrorMessage extends StatelessWidget {
final String? title;
final String? message;
final String? actionText;
final VoidCallback? action;
final List<({String text, void Function() action, bool loading})>? actions;
Copy link
Member

Choose a reason for hiding this comment

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

Interesting use of records!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's close to needing a class. 🫣

@micahmo
Copy link
Member Author

micahmo commented Apr 16, 2024

this is ready to be merged?

If the last screenshot looks good to you, yes!

@hjiangsu hjiangsu merged commit 571b3bf into thunder-app:develop Apr 16, 2024
1 check passed
@micahmo micahmo deleted the fix/loading-issue branch April 16, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants