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

Improved feedback when subscribing/unsubscribing to community #1499

Merged
merged 3 commits into from
Jul 17, 2024

Conversation

hjiangsu
Copy link
Member

Pull Request Description

This is a super small PR which adds snackbar messages for subscribing and unsubscribing from communities.

Issue Being Fixed

Issue Number: #1423

Screenshots / Recordings

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-16.at.09.39.05.mp4

Checklist

  • If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
  • Did you use localized strings (and added appropriate descriptions) where applicable?
  • Did you add semanticLabels where applicable for accessibility?

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

LGTM

if (event.value) {
showSnackbar(AppLocalizations.of(GlobalContext.context)!.subscriptionRequestSent);
} else {
showSnackbar(AppLocalizations.of(GlobalContext.context)!.unsubscriptionRequestSent);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need this one since I don't believe unsubscripions have the same delay as subscriptions where the can be in pending for a while. But I'll leave that up to you!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not actually too sure how the pending states work in this situation, so I just applied it to both! I can remove the unsubscription one though since you may be correct here!

If you know of a way to trigger the pending state, let me know and I can test it out

@micahmo
Copy link
Member

micahmo commented Jul 16, 2024

What happens if you test this on an instance that isn't yours? Do you get the "subscription request sent" message only?

@hjiangsu
Copy link
Member Author

What happens if you test this on an instance that isn't yours? Do you get the "subscription request sent" message only?

It'll still show both the subscription request and the confirmation afterwards (similar to the video demo)

@micahmo
Copy link
Member

micahmo commented Jul 16, 2024

Here's my understanding of how things work.

  • On your own instance, requesting to subscribe always returns that you are immediately subscribed (so we shouldn't need the "request sent" message here).
  • On any instance, requesting to unsubscribe always returns that you are immediately unsubscribed (so we shouldn't need the "request sent" message here either).
  • However, when subscribing to a community on another instance, it always returns pending. In the past, we had put in a trick (here) to do another API request after a delay, with the idea that most of the time, if we checked again after a second, the status would be updated from pending to subscribed. Although more recently I've noticed that this isn't always the case and I have to manually refresh after a significant amount of time before it returns that I am subscribed.

I think what I would recommend is...

  1. Check the return value from followCommunity. If it matches what we expect for the given event, then just tell the user immediately they they're subscribed or unsubscribed.
  2. If it doesn't match what we expect for the event, then show the "request sent" message. And only then, do the one-second delay (or maybe longer) and check again. If the result matches after the second check, show the final message to the user. If it still doesn't match, I think we do nothing and let the user manually refresh or check later.

What do you think?

@hjiangsu
Copy link
Member Author

Thanks for the response! I think I did pretty much what you suggested but with a slight change (I didn't see your message before 😅)

The logic goes like this:

  • We'll always send the "subscription requested" message when we attempt to subscribe to a community regardless of the instance. This happens before followCommunity is triggered, because we don't know how long that API call will take (depending on network conditions)
  • From the result of followCommunity, we'll return back the subscribed/unsubscribed message immediately if it matches what we are expecting from event.value
  • If it doesn't match (e.g., its in the pending state or it hasn't properly updated), then there's a delay of 1 second before we attempt to fetch again. If it works the second try, then display the subscribed/unsubscribed message to the user. Otherwise, we don't display anything

Here's some demo videos to show the differences:

Subscribing to local community (very slow connection):

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-16.at.15.02.34.mp4

Subscribing to local community (fast connection):

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-16.at.15.03.15.mp4

Subscribing to federated community:

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-16.at.15.04.14.mp4

Unsubscribing from federated community:

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-16.at.15.05.16.mp4

Unsubscribing from local community (slow connection):

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-16.at.15.06.11.mp4

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

Haha we were on the same wavelength. This looks perfect, thank you!!

@hjiangsu hjiangsu merged commit e02bf30 into develop Jul 17, 2024
1 check passed
@hjiangsu hjiangsu deleted the feature/subscription-snackbar branch July 17, 2024 15:23
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