-
Notifications
You must be signed in to change notification settings - Fork 257
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: add a "mark notification as done" button #706
Conversation
Also, on a side note, I have some other ideas for improvements that are now possible thanks to this new API endpoint:
LMK what you think about it, I'd gladly work on that too! |
Your comments about read vs done are interesting. In my workflow, I generally treat "read" as "done." In your workflow, what's the difference between the two? Edit: Actually, could you please split that discussion to a new Issue? That will keep this PR focused on a single topic (which already expands into discussions about positioning of icons etc). |
Re: the changes here -- it seems natural to mirror the Github behaviour and icons. Looks like those don't have a read icon, actually. Opening the entry marks it as read. And I guess Done probably marks it as read + done? Maybe we could match that, which means only two icons. (This also means I support changing the icon to match Github's interface.) If we do keep all three icons, then in a row as suggested at the bottom of the description looks best to me. @afonsojramos What do you think? |
BTW @adufr -- thanks for the contribution! |
Yup I'll open 2 different issues, one for each matter |
expect(result.current.isFetching).toBe(false); | ||
}); | ||
|
||
expect(result.current.notifications.length).toBe(0); |
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.
This seems odd. If the request failed, shouldn't the notification list remain untouched, length 1? My read of the code under test is that it doesn't change the notifications list in the failure condition, but maybe I'm wrong.
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.
I agree, tbh I just copy pasted the markNotification
tests, so they were already broken before my PR 😬
I'm trying to figure out how to initialize a non-empty notifications array with mocked data, but without any success for now...
Great contribution @adufr. @bmulholland @adufr your comment above got me thinking....
Does this mean that the setting |
I've only recently picked up contributing here, so I'm not familiar with all the options. I wasn't considering that Setting. It does kinda feel like this read vs done workflow replaces the need for that. But I don't know what people actually use, and I suppose there's lots of possible workflows. We could take an opinionated approach to it but it would require deep consideration and that's beyond the level of effort I'm able to put in. One easy take is this "mark as done" would replace the "mark as read," though. I honestly don't think most people know or care about the distinction, so having one button for mark as done, no button for mark as read, and keeping the setting seems like a reasonable middle ground. |
From my observations, changing Is that the same experience you observe? |
Maybe But the EDIT: see #708 for the new feature request |
Re: "Mark as read on click" -- I confirm this has no effect now, Github marks it as read when you open it anyway. We should remove the Setting. Re: a "mark as done on click" -- When opening the notification, you get a blue bar at the top where you can click Done. That's two clicks, but it's also how Github does it, so maybe good enough? |
Why not add the setting anyway? For those who, like me, might be happy to have it? |
Sure, especially considering we just removed another setting :) |
Well, first of all great contribution @adufr! It sparked quite a lot of discussions so it is not even just about the individual contribution, thank you! Sorry for coming late to the party, but these past days have been a bit hectic.
Totally agree here! As now we've removed the "mark as read" I'd say that marking it as done on click would definitely be an option I would at least consider enabling to be honest. Everyone has a different workflow, so I'm up for it.
GitHub doesn't, but we probably could I believe... Seems like an easy add to just traverse across all of the repo notifications and mark them as done. |
Thanks!
Yes I agree, I thought about it! but we should document ourselves about rate-limits just to be safe... |
Okay, to keep us moving, how about we merge this PR, and agree on next steps (follow up PRs)? Are you cool with that @afonsojramos ? In that case, @adufr , only thing before merge is to get the branch green and conflict-free. Those follow-on PRs are, I believe:
Have I missed anything? |
Hi! Sorry I've been sick as hell for the past week! So to clarify, IMO this PR is still valid as-is (I've just updated it with
|
On my end, I'm good to merge this -- even if there's some details wrong, better to keep moving. Just one code thing: what about #706 (comment) ? |
Tests and build fail though. I'd prefer to have that fixed before merging. |
Sorry it should be good now! |
@afonsojramos I missed that, and I agree. Thanks! |
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.
LGTM. @afonsojramos good with you? Go ahead and merge if you're okay with it
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.
Looks good to me! Sorry for the late review!
Related issues
#500 - feature request: mark notifications as done
Context
Hi! This is my first PR on this repo; let me thank you for making this great app!
As a new user, I instantly found quite frustrating that I wasn't able to mark notifications as done. After doing some digging, I found that GitHub does now provide an endpoint for that 🎉
Discussion
This PR changes the existing "mark notification as read" icon, to better reflect what it does, and adds a new "mark notification as done" button right next to it:
![image](https://private-user-images.githubusercontent.com/26418696/297668297-0c55156a-b297-468f-9d4f-f8cb37e32a2c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU1OTYyNzAsIm5iZiI6MTcyNTU5NTk3MCwicGF0aCI6Ii8yNjQxODY5Ni8yOTc2NjgyOTctMGM1NTE1NmEtYjI5Ny00NjhmLTlkNGYtZjhjYjM3ZTMyYTJjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA5MDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwOTA2VDA0MTI1MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWYyMGM0Zjc3ZTY0MWY4N2E2YzVkMTAzYmFkMjFjYzJjZGM4ZTZiNjkzMDI3NjIyNjQzNjFlYjg5NDk3NjAzY2MmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.xjRl2E72hHPnW0igiDA0aMRXIpkkDGF4zzSl0_PbBMI)
Unfortunately, it looks like GitHub does not provide an endpoint for marking all notifications from a repository as done, so I couldn't add a new button to the Repository component 😞
Also, if that's OK with you I'd like to change the "Unsubscribe" button:
Let me know what you think about it; I can do that in another PR if you prefer so 🤷