Skip to content

feat: notification user avatar #915

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

Merged
merged 18 commits into from
Apr 6, 2024
Merged

feat: notification user avatar #915

merged 18 commits into from
Apr 6, 2024

Conversation

setchy
Copy link
Member

@setchy setchy commented Mar 21, 2024

Took this idea from the GitHub Android application...

Display last updated user avatar. Users name is available when mousing over.

Screenshot 2024-03-20 at 10 51 07 PM

@setchy setchy added the enhancement New feature or enhancement to existing functionality label Mar 21, 2024
@setchy setchy added this to the 5.1.0 milestone Mar 21, 2024
@setchy setchy marked this pull request as ready for review March 21, 2024 04:23
@setchy setchy marked this pull request as draft March 21, 2024 04:24
@adufr
Copy link
Contributor

adufr commented Mar 25, 2024

This is a really cool addition!

I see that the PR is still in draft? Is there something missing to make it ready for review?

@setchy
Copy link
Member Author

setchy commented Mar 25, 2024

Still in draft. I need to overhaul the User-related types first.

@setchy setchy marked this pull request as ready for review April 3, 2024 06:17
@setchy
Copy link
Member Author

setchy commented Apr 3, 2024

Took a bit of git wrestling to get this branch back up-to-date...

appreciate feedback on the layout of the profile icon. i played around with it being on the far left, but wasn't sure where the Subscribed, Mentioned notification reasons would be best displayed then...

bmulholland
bmulholland previously approved these changes Apr 3, 2024
@setchy
Copy link
Member Author

setchy commented Apr 3, 2024

How do we feel about the structure of that footer.

It will follow two patterns, depending on whether the notification event contains user data.

  1. <reason> - <avatar img> updated about <some time>
  2. <reason> - Updated about <some time>

Some open questions that are going through my mind

  • should the avatar image be at the start of the notification. if so, where do we put the reason?
  • should we have a default avatar image for unknown user. that would ensure each row has a consistent shape. currently it can vary depending on the notification content (example below)

Screenshot 2024-04-03 at 11 30 02 AM

@adufr
Copy link
Contributor

adufr commented Apr 3, 2024

I like that there's no avatar when the reason isn't a user (like for the worflow run notification in your last screenshot); it helps me quickly identify different kind of notifications

I don't really have an opinion on the placement of the avatar, it doesn't look too bad like this...
Or maybe having it at the end (like, "updated about 4 hours ago by <avatar>)..? I don't know, it looks ok to me like this!

@bmulholland
Copy link
Collaborator

I kinda like the avatar at the start, makes it more like an english sentence, "actor acted at time"

@setchy
Copy link
Member Author

setchy commented Apr 4, 2024

I kinda like the avatar at the start, makes it more like an english sentence, "actor acted at time"

I'll mock up an example of what this may look like.

@setchy setchy marked this pull request as draft April 4, 2024 14:12
@setchy
Copy link
Member Author

setchy commented Apr 5, 2024

Examples of what an alternate layout could look like.

Format is <avatar> <reason type> <update at>, where

  • avatar is user avatar when known, else fallback to feed person octicon
  • reason type used instead of previous hardcoded updated. Each reason.type will be updated to be correct tense (ie: Mention -> Mentioned, etc)
  • updated at, no change

Screenshot 2024-04-05 at 1 17 59 PM
Screenshot 2024-04-05 at 1 18 10 PM

My 2 cents - this feels much more natural

@setchy setchy marked this pull request as ready for review April 5, 2024 17:25
@setchy setchy requested a review from bmulholland April 5, 2024 17:25
@bmulholland
Copy link
Collaborator

I love those, clean and clear :)

@adufr
Copy link
Contributor

adufr commented Apr 5, 2024

Looks great!

@setchy setchy dismissed bmulholland’s stale review April 5, 2024 18:08

would like a fresh approval after the ux-pivot

@afonsojramos
Copy link
Member

Looks awesome!! Let me run this locally to check it out better!

@setchy
Copy link
Member Author

setchy commented Apr 6, 2024

Looks awesome!! Let me run this locally to check it out better!

Thanks mate. Appreciate any feedback, particularly on the changes to the new reason mappings.

Copy link
Member

@afonsojramos afonsojramos left a comment

Choose a reason for hiding this comment

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

We lose the usernames, but we get the avatars, I think this is a positive change overall, love it!

Clean implementation as well, all wins!

@setchy
Copy link
Member Author

setchy commented Apr 6, 2024

We lose the usernames, but we get the avatars, I think this is a positive change overall, love it!

Clean implementation as well, all wins!

thanks @afonsojramos, appreciate the feedback. usernames still available on avatar hover :)

@setchy setchy merged commit c1ebda3 into main Apr 6, 2024
7 checks passed
@setchy setchy deleted the feature/user-avatar branch April 6, 2024 21:06
@afonsojramos
Copy link
Member

We lose the usernames, but we get the avatars, I think this is a positive change overall, love it!

Clean implementation as well, all wins!

thanks @afonsojramos, appreciate the feedback. usernames still available on avatar hover :)

I know, but we still lose them at a glance, which is mostly what matters.

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