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

Fix image avatar alignment in notifications #2906

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Jun 7, 2021

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

image

After

image

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@davwheat davwheat self-assigned this Jun 7, 2021
Copy link
Member

@SychO9 SychO9 left a 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 ?

@davwheat
Copy link
Member Author

davwheat commented Jun 11, 2021

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

@SychO9
Copy link
Member

SychO9 commented Jun 11, 2021

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.

It doesn't though because of how the notification grid is structured.

@davwheat
Copy link
Member Author

If the title is multi-lined, it does.

image

@SychO9
Copy link
Member

SychO9 commented Jun 11, 2021

If the title is multi-lined, it does.

image

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 .Avatar to middle.

@davwheat
Copy link
Member Author

davwheat commented Jun 11, 2021

I'd agree with something like that, but it could break 3rd party styles, annoyingly :(

Copy link
Member

@SychO9 SychO9 left a 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

@davwheat davwheat merged commit d1e3855 into master Jun 11, 2021
@davwheat davwheat deleted the dw/2905-fix-img-avatar-alignment branch June 11, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notification avatar alignment issue
3 participants