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

feat: show read notifications #889

Closed

Conversation

setchy
Copy link
Member

@setchy setchy commented Mar 16, 2024

closes #708

Add read / unread toggle in sidebar. Read notifications have a higher opacity and hide the mark as read button on hover-over.

Screenshot 2024-03-18 at 8 10 19 AM

Some GitHub API Limitations I've found:

  • No endpoint to mark a read notification as unread
  • No attribute in the /notifications response to indicate if a notification is done or not done (ie: in the inbox)

These have been documented in #890

@setchy setchy changed the title Feature/read unread notifications feat: show read notifications Mar 16, 2024
@setchy setchy added the enhancement New feature or enhancement to existing functionality label Mar 18, 2024
@setchy setchy modified the milestones: Release 5.2.0, Release 5.3.0 Mar 29, 2024
@setchy
Copy link
Member Author

setchy commented Apr 3, 2024

With the improved Settings layout, I've kept this consistent and made this a new Notifications setting called Show read notifications

Screenshot 2024-04-03 at 12 45 52 AM

Example
Screenshot 2024-04-03 at 12 47 39 AM

The feature is a little limited since we don't fetch more than the first page of notifications (per account). Later, this may change

@setchy setchy marked this pull request as ready for review April 3, 2024 04:48
@@ -23,11 +23,11 @@ describe('hooks/useNotifications.ts', () => {
];

nock('https://api.github.com')
.get('/notifications?participating=false')
.get('/notifications?all=false&participating=false')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needing to update the same string throughout is a code smell, FWIW :)

Copy link
Collaborator

@bmulholland bmulholland left a comment

Choose a reason for hiding this comment

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

These features are getting easy :). Nice stuff!

@setchy
Copy link
Member Author

setchy commented Apr 3, 2024

These features are getting easy :). Nice stuff!

Haha yeah, they're starting to flow nicely.

I remembered a use-case I need to test with this enhancement (ensure clicking mark as read correctly changes row state before the next scheduled fetch). Will confirm shortly

@bmulholland
Copy link
Collaborator

If you're happy and you know it, merge conflict

@setchy
Copy link
Member Author

setchy commented Apr 3, 2024

Moving back to DRAFT as there is some more changes required to ensure that when showReadNotifications is enabled that the notification row doesn't disappear, etc

@setchy setchy marked this pull request as draft April 3, 2024 15:01
@setchy setchy removed this from the Release 5.3.0 milestone Apr 11, 2024
@setchy
Copy link
Member Author

setchy commented Apr 19, 2024

Reopening a new PR (#1052) as I've reworked this

@setchy setchy closed this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: add a "Show read notifications" setting
2 participants