feat(github-feed): add GitHub notifications#218
Conversation
Swarsel
force-pushed
the
feat/notification-count
branch
from
283dfc4 to
4d0c724
Compare
I'll review it out in a little while, but as initial feedback for the future don't apologize! In any case, apologizing beforehand is too unprofessional. |
|
You're showing the notification in the TOP of the feed? Can you show how this looks with the entire feed? |
I disagree, but let's not get sidetracked!
Yes they are rendered on top: 
Notifications are usually things I want to see and address quickly, so personally I would prefer it like that instead of interweaving them with the feed, which might have many entries. |
|
The idea is not bad, it is even useful. But the implementation changes the concept of the plugin too much. The intention of the plugin is mainly to "mimic" the github.com/feed or github.com/dashboard-feed. I think mainly your implementation should not change that. What I suggest you to do is to separate the notification from the feed. It can be in two tabs, for example. And don't change the UI of it. Adding a border seems pretty ugly to me |
Swarsel
force-pushed
the
feat/notification-count
branch
7 times, most recently
from
83da87c to
e2c83bd
Compare
|
Yeah, I agree that is the cleaner approach. Implemented now, along with a setting that lets the user decide which tab to open by default (per default the "Activity" tab will be shown first, so the only change for existing users is the tab bar):
|
|
Also I went ahead and added an optional badge to the widget that shows the number of notifications: That badge can be configured to be not shown at all, and also it's colors can be customized according to the theme, similar to how other other bar widgets do it, e.g. Control Center |
Swarsel
force-pushed
the
feat/notification-count
branch
from
f5b8b12 to
5fb9727
Compare
|
As a last addition, I added optional system notifications for notifications and all supported types of events: Each of those types is separately toggleable With this last addition, all the functionality that I would like to have in this plugin is now implemented. Would be great if you could take another look @linuxmobile! |
Thx, now looks better. I will check tonight! |
|
tag me when if/when it's time to merge @linuxmobile |
|
@Swarsel What is your status? Are you going to make the modifications? |
|
What modifications? From my standpoint this PR is good to go. |
|
|
Ah, I think you forgot to hit the "Finish your review" button - currently I cannot see your comments :) I believe you can still do so now, it should not be lost |
| ListView { | ||
| id: eventsList | ||
| model: root.events | ||
| ColumnLayout { |
There was a problem hiding this comment.
Is there a justification of why you change from ListView to ColumnLayout?
There was a problem hiding this comment.
@Swarsel I've already reviewed the PR, and this is still shocking to me. I would like to know why you decided to change from ListView to ColumnLayout?
There was a problem hiding this comment.
I was under the impression that this was the easier approach for realizing the tabbed layout for activities and notifications; at the moment I am not sure how the solution would look using ListView but I am also not too knowledgable about QML layouting.
Swarsel
force-pushed
the
feat/notification-count
branch
from
5fb9727 to
50de997
Compare
|
@linuxmobile - this PR modifies your plugin. Please review when you have a chance. |
Swarsel
force-pushed
the
feat/notification-count
branch
from
50de997 to
0232d96
Compare
|
@linuxmobile - this PR modifies your plugin. Please review when you have a chance. |
|
I don't have enough energy to continue with the review, I mark it as approved @ItsLemmy. |
3 participants
Hey, thanks for creating this plugin, I love it a lot!
While I like to have the feed information, another bit of information that I also value a lot is information on the count of notifications that my account has.
Hence I added notification fetching capabilities to the github-feed plugin; when enabled and the token has the correct scopes, it will fetch them as well.
Preview:
Do note that I vibe-coded most of this, this is not a stellar implementation. I just quickly wanted something that gets the job done, and this works. Also I realize that notifications might be out of scope of this plugin; if either of these are a problem then I am sorry for the disturbance :)