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: update/move "unsubscribe" icon #745

Merged
merged 11 commits into from
Feb 6, 2024
Merged

feat: update/move "unsubscribe" icon #745

merged 11 commits into from
Feb 6, 2024

Conversation

adufr
Copy link
Contributor

@adufr adufr commented Feb 6, 2024

Context

As discussed in #706, because of the new "Mark as done" button, the "unsubscribe" button was weirdly positioned, so this PR fixes it by moving icons

I also updated the "unsubscribe" icon to match Github's one (bell-slash)

Before:
image

After:
image

Discussion

I changed the order of the icons to something that felt more natural to me / something that looks more like Github's notifications interface (even though they don't provide a "mark notification as read" button).

I guess this is kind of subjective so if you see reasons to not change it, please let me know 😛

@adufr
Copy link
Contributor Author

adufr commented Feb 6, 2024

Uh oh, idk what happened with my branch management but I was only expecting 2 commits here, not 10, I might have been doing something wrong -- looks like I probably didn't correctly update my fork's main branch 😬

Anyway, the files changed are looking good to me 🤷

@setchy
Copy link
Member

setchy commented Feb 6, 2024

Looks much cleaner @adufr - thank you

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.

Yeah that's better -- thank you!

@bmulholland bmulholland merged commit cfaf623 into gitify-app:main Feb 6, 2024
@adufr adufr deleted the feat/notification-row-icons branch February 6, 2024 18:13
@setchy
Copy link
Member

setchy commented Feb 15, 2024

@adufr - with this now merged, I've been using this main branch today to do some other testing.

What i noticed with the new UX re: the 3 icons, is the following

  • previously, the mouse hit-spot for the mark as read icon was the full row height. now, it's only a portion of the row height
  • previously, all of the mark as read icons were vertically aligned to the right, both for repository and notification rows. this made going through the list really easy. now, there is some variance horizontally for the mark as read icons which
Screenshot 2024-02-15 at 5 18 54 PM

i'd like to propose we consider:

  • placing the mark as read on the far right
  • increasing it so that it's hit-spot is the full row height
  • aligning vertically

Would love to hear your thoughts and experience using it thus far.

@adufr
Copy link
Contributor Author

adufr commented Feb 16, 2024

@setchy I haven't used it much since I'm only running it while working on it, otherwise I'm still on 4.6.1..

I agree on moving the "Mark as read" icon to the right, which will allow us to align the icon with the "Mark repository as read" icon (which will need to be a bit smaller)

And concerning the button height, I'm proposing to make it h-full, while still staying inside the NotificationRow's padding, this way it's a bit easier to click on the button, but it also prevents you from accidentally clicking the button of the next/previous notification row

Here's what it would look like (I'm about to open the PR):
image

@adufr adufr mentioned this pull request Feb 16, 2024
@bmulholland
Copy link
Collaborator

Thank you both for the attention to detail! Both the testing and the fixes are invaluable for the app to remain polished and usable. Much appreciated.

@afonsojramos
Copy link
Member

Interesting analysis there, thanks for the keen eyes 👀

@setchy setchy added the enhancement New feature or enhancement to existing functionality label Mar 27, 2024
@setchy setchy added this to the Release 5.0.0 milestone Jul 17, 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.

4 participants