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

[Notifications] Add a Mentions tab #7044

Merged
merged 25 commits into from
Dec 12, 2024
Merged

[Notifications] Add a Mentions tab #7044

merged 25 commits into from
Dec 12, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 11, 2024

This adds a new Mentions tab to the Notifications screen. This tab is equivalent to the Mentions tab on Twitter. It filters out non-post notifications (follows and likes) but displays every post in your notifications (including replies, mentions, and quotes). The previous behavior is now a tab called All (which remains the default on the Notifications screen).

Note that this does not filter out any posts. On the contrary, it lets you read only notifications that are posts. The All tab did not work very well for that because it fetches likes and follows, causing posts (including quotes, mentions, and replies) to be easily lost behind the pagination boundary. Unlike All, the Mentions tab will only display posts (including quotes, mentions, and replies) so it should reduce the likelihood of missing replies.

Here's a screenshot of two tabs being displayed:

Screenshot 2024-12-11 at 19 13 24

I'm not showing the Mentions tab because we don't actually have the backend wired up for it yet so I can't test whether it will work. However, you can imagine it will display only the posts.

  • UI scaffolding
  • Query scaffolding
  • Upgrade the bindings Add reasons param to listNotifications atproto#3222
  • Get the backend change out? Filtering on listNotifications atproto#3225
  • Verify that polling, refreshing, and invalidation logic makes sense
  • Fetch lazily
  • Verify each usage of RQKEY for whether it should do both or just 'all'
  • Verify no regressions to the All tab
  • Persist selection (for now, just for the session)
  • Expand tab widths?
    • Meh. Might follow up. This is already chunky.

Reviewing

I don't have a great suggestion other than testing it. You can review commit by commit to trace the line of thinking, I tried to keep those digestible. Reading through the entire code isn't too bad either.

Some things to pay attention to:

  • This should be 100% independent from the All tab mechanism. It's a separate simple RQ cache etc. It doesn't try to be smart about read states or counters. To reset the counter and to mark things as read, you still have to go to the All tab — which I believe is how it works on Twitter as well.
  • It retains the selected tab, but only during the session. This is so you don't accidentally get stuck forever on the Mentions tab. In case you don't realize how to clear the badge. If we later make it more obvious, we could change this.
  • Since we don't mark things as read on the Mentions tab, we also don't highlight unread items. A bit weird but ok.
  • The header spinner shows the state of the currently selected tab. Soft reset refreshes the current tab. Etc.
  • The Mentions tab has no polling and no "has new" indicator. So you won't know when there's a new mention — but the bell still works and pressing still scrolls up (and refreshes) so it would refresh Mentions as well.
  • I opted to keep the tabs always visible for now. Could maybe make them sticky but not sure.
  • I'm not completely in love with the visual appearance of the pager here due to "All" being very short. I think the real fix would be to actually fix the pager to expand items when there's few of them. I want that for Home too. So I might work on that separately from this PR but this shouldn't block it.

Copy link

github-actions bot commented Dec 11, 2024

Old size New size Diff
6.77 MB 6.77 MB 0 B (0.00%)

@Sininini
Copy link

Sininini commented Dec 11, 2024

I apologise for commenting with a feature request here, but since features aren't often revisited I'd like to ask here.
It would be really good to be able to change the default tab. Accounts with lot of engagement likely don't care about individual likes and reposts much, so it would be nice for them to see comments with less ux friction.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 11, 2024

I think we could just remember the selection across reloads which would solve that

@surfdude29
Copy link
Contributor

It's great to see this being worked on, as I think it will be a big QoL improvement for so many users!

... Conversations seems like a more precise name, but I'm open to calling it Mentions if there are strong feelings about that.

Conversations does seem like a more precise, intuitive name to me (replies and quotes are just as important to see as mentions) and so I think it will better convey the meaning of what notifications it shows, especially to new users, some of whom may not be familiar with Twitter.

People may still refer to it as "my mentions" in casual usage, but we'll know what they mean. (Or maybe they'll start saying "my convos"?)

I think we could just remember the selection across reloads which would solve that

This would be excellent, as I think I would probably spend most of my time in the Notifications screen in the Conversations tab, maybe occasionally checking out All, so it would be very useful if the selection were to be remembered across reloads.

Expand tab widths?

I don't have a particularly strong view on this, I think it looks fine as-is, but maybe it would look better with bigger tabs, like Twitter? Fwiw one of my followers commented that they'd prefer that it look that way.

I also have one feature request, although I don't know how technically feasible it would be: what about showing follow back notifications as well in the Conversations tab? Yes, they're not strictly speaking Conversations, but I think they're more valuable info to most users compared to general follower notifications from people you probably don't know, as you've already indicated that you're interested in that account by following it.

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 11, 2024

I changed to Mentions to reduce bikeshedding, for familiarity, and because we already use the word "Conversations" in DMs.

@arcalinea arcalinea temporarily deployed to convos-tab - social-app PR #7044 December 12, 2024 05:07 — with Render Destroyed
@gaearon
Copy link
Collaborator Author

gaearon commented Dec 12, 2024

talked to @pfrazee, merging to test

@gaearon gaearon merged commit f8cdd6b into main Dec 12, 2024
6 checks passed
@gaearon gaearon deleted the convos-tab branch December 12, 2024 17:37
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.

4 participants