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

[🔢] Unseen activity count #533

Merged
merged 6 commits into from
Jun 4, 2019
Merged

[🔢] Unseen activity count #533

merged 6 commits into from
Jun 4, 2019

Conversation

eoji
Copy link
Contributor

@eoji eoji commented May 31, 2019

What ❓

  • Displaying logged in user's unseen activity account in drawer with tests.
  • Created reusable styles for drawer items with counts.
  • Closing the drawer when navigating to activity feed.
  • Updated schema to access clearUserUnseenActivity mutation.
  • Refreshing user after clearing unseen activity and tests.

How to QA? 🤔

Well, if you have some unseen activity, the drawer hamburger should show and indicator and the Activity row in the drawer should show the count.

Story 📖

Trello Story

See 👀

I had the foresight to take a screenshot before I cleared it 😎

@eoji
Copy link
Contributor Author

eoji commented May 31, 2019

@eoji eoji requested a review from justinswart May 31, 2019 22:47
Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Logic seems sound as far as I can tell! Just had two questions. Will leave for an Androider to approve ✅😄

.compose(bindToLifecycle())
.subscribe(this.surveys);

loggedInUser
.compose(takeWhen(refreshSurvey))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why this is called refreshSurvey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, I think a better name could be resumeOrRefresh

.switchMap(__ -> this.apolloClient.clearUnseenActivity().compose(neverError()))
.switchMap(__ -> this.apiClient.fetchCurrentUser().compose(neverError()))
.compose(bindToLifecycle())
.subscribe(freshUser -> this.currentUser.refresh(freshUser));
Copy link
Contributor

Choose a reason for hiding this comment

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

So just to confirm I am reading this correctly:

  1. User refreshes feed.
  2. Take the current user and inspect their unseenActivityCount.
  3. If this is not zero, clear the activity count and refresh the user.

I guess the only edge-case race condition is if, between the user logging in and looking at the feed, new activities come in. Would they be alerted to this by the OS via the push notification in some way? I'm also kind've wondering how to handle this on iOS 🤔

Copy link
Contributor Author

@eoji eoji Jun 3, 2019

Choose a reason for hiding this comment

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

Yeah that's how it works. And hmm, they would get the push but no, we don't refresh the users on pushes. Should I maybe do a combineLatest like

refreshSurvey
. combineLatest(currentUser)
.filter(ObjectUtils::isNotNull)
//do the check and refresh

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh also I think the situation with log in would be.
They're logged out.
They log in.
loggedInUser emits.
They return to that screen. onResume is fired (a lifecycle method).
So we'd have the updated user.

But that only handles log in, not getting pushes or new activity while on that screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if a push comes in while the user is in the app we should refresh the count, but if the user is on the activities view while that happens then I guess we can ignore it, seeing as we'd have to immediately clear it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke to @nnekab! She said for now, Android doesn't need to update the count after a push comes in because you can't actually see the number on that screen. It's only in the drawer in Discovery.

Copy link
Contributor

@Scollaco Scollaco left a comment

Choose a reason for hiding this comment

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

lgtm! 🎉

@eoji eoji merged commit 02add4a into master Jun 4, 2019
@eoji eoji deleted the activity-count branch June 4, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants