-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Fix image avatar alignment in notifications #2906
Conversation
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.
Any reason for doing this instead of changing align-items: baseline;
to align-items: center;
in .Notification
?
Doing center will make the avatar centrally aligned on multi-line notifications, which doesn't match the style in previous versions, or the style of the text-based avatars. Instead, we want the avatar in line with the first line of notification text |
It doesn't though because of how the notification grid is structured. |
Ah, I see what you mean now, it's because the image itself is a cell of the grid that we can't just change its vertical alignment to middle. Another way of fixing this would be to wrap the notification avatar img with a div or span, then setting the vertical alignment of the |
I'd agree with something like that, but it could break 3rd party styles, annoyingly :( |
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.
True, we could do that for 2.0, but better avoid for 1.x
Fixes #2905
Changes proposed in this pull request:
Fixes issue introduced with #2822 where image-based avatars would display misaligned in the notifications list.
Reviewers should focus on:
Are there any other issues with Notification avatars? Can anyone think of a less hacky way to achieve this?
Screenshot
Before
After
Confirmed