-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
adding notification filters #3612
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! This is quite overwhelming and I think this level of granularity isn't that useful. How about we start with All and Mentions, similar to Twitter? Mentions would include any textual interactions, i.e. both replies and quotes. |
Thanks @gaearon! I agree. I've gone ahead and updated the PR. |
This implementation seems to just replace the contents of the same list, but I think we should treat these as actually separate tabs. E.g. you don't want scrolling one to affect the other. You can check the Search page for an example of how to make it work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to use a real Pager
OK, so the experience here isn't super great because all filtering is done client-side, and so the "mentions" tab may require multiple roundtrips to get anything on the next page (and it may just be a few items). I think this makes it not super great since for large accounts, most notifications would probably be non-mentions anyway so the feature won't solve the problem they're having. We need to add a separate backend endpoint for this. I'll add it to a roadmap but probably not super soon. |
Sounds good, thanks @gaearon! |
Following on from this, there's now a draft PR open to add a Mentions tab to the Notifications screen: |
This PR is meant to be a jumping point for adding filters to the notifications screen to sort by type.