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: support grouping notifications by date or repository #854

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

AltinGruda
Copy link

Added a dropdown menu to group notifications by date or repository. UI changes were made based on how it looks on https://github.com/notifications

closes #836

@setchy
Copy link
Member

setchy commented Mar 3, 2024

Thanks for the contribution @AltinGruda. Functionally it seems to do the trick.

I'm wondering whether the toggle should either be in the Settings menu, or, on the left-hand-side, perhaps below the notification count bell.

@brandonweiss @afonsojramos - what do you both think?

@AltinGruda
Copy link
Author

AltinGruda commented Mar 3, 2024

I thought of putting it in the sidebar below the notification bell icon, with the icon shown below. Let me know what you guys think.
image

@setchy
Copy link
Member

setchy commented Mar 4, 2024

I thought of putting it in the sidebar below the notification bell icon, with the icon shown below. Let me know what you guys think.
image

Let's see what that looks like

@AltinGruda
Copy link
Author

AltinGruda commented Mar 4, 2024

This is how it looks with the dropdown being below the notification icon.

image
image

@setchy
Copy link
Member

setchy commented Mar 5, 2024

Thanks @AltinGruda - certainly looks better on the left side.

A suggestion I'd appreciate feedback on... What if instead of the drop-down we simply toggled between two icons, with suitable hover text?

@afonsojramos
Copy link
Member

What if instead of the drop-down we simply toggled between two icons, with suitable hover text?

@setchy, I understand where you're coming from, but in filters world, that is not a common UX practice, and thus, I would stay away from it.

@setchy setchy added the enhancement New feature or enhancement to existing functionality label Mar 5, 2024
@setchy
Copy link
Member

setchy commented Mar 5, 2024

What if instead of the drop-down we simply toggled between two icons, with suitable hover text?

@setchy, I understand where you're coming from, but in filters world, that is not a common UX practice, and thus, I would stay away from it.

To me, it's really just a yes / no group by repository, not filtering at all :)

If we're not keen on that, perhaps we add it into the Settings menu along with our other options.

@afonsojramos
Copy link
Member

Right, but in the future maybe we will implement other sorting/grouping capabilities within the same menu, hence why I think the way it is looks better

@AltinGruda
Copy link
Author

I've considered the suggestion to turn the filter into a toggle and created a preview (screenshots attached, have not pushed the changes). While toggles can offer a clear interface, I'm open to feedback on whether this aligns with our UX goals or if there are better alternatives.

image
image

@setchy setchy changed the title feature: support grouping notifications by date or repository feat: support grouping notifications by date or repository Mar 6, 2024
@AltinGruda
Copy link
Author

Are we sticking with the dropdown or the toggle one?

@afonsojramos
Copy link
Member

afonsojramos commented Mar 11, 2024

I'd go with the dropdown, but the implementation needs fixing. Clicking outside of the dropdown should close the dropdown, not block the whole app from interaction.

@AltinGruda
Copy link
Author

I went ahead and implemented the functionality to close the dropdown when clicking outside of it. The issue where clicking outside the whole app closes seems to be on the main branch as well, so I dont think I caused that issue

Copy link
Member

@setchy setchy left a comment

Choose a reason for hiding this comment

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

@AltinGruda - this is looking pretty close.

I've noticed that when using the Date view, that the notifications aren't being correctly ordered by date (newest to oldest) which is the GitHub UI behavior - https://github.com/notifications

It seems that currently just hiding the repository header row....

Additionally, do you think it is possible to add something that indicates the currently selected filter option?

@setchy setchy marked this pull request as draft April 5, 2024 15:30
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: support grouping notifications by date or repository
3 participants