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(UI): Make post listing (icon) avatars smaller (line-height) #1597

Merged
merged 7 commits into from
Jun 27, 2023

Conversation

jsit
Copy link
Contributor

@jsit jsit commented Jun 25, 2023

Need opinions on this. This will reduce some visual noise and also tighten things up vertically a little.

Before

Screenshot 2023-06-25 at 4 37 21 PM

After

Screenshot 2023-06-26 at 6 06 42 PM Screenshot 2023-06-26 at 6 09 19 PM

Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

I think it looks better. Waiting for others' feedback before merge.

Copy link
Contributor

@alectrocute alectrocute left a comment

Choose a reason for hiding this comment

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

I think it looks better, too.

@jsit
Copy link
Contributor Author

jsit commented Jun 25, 2023

@alectrocute @SleeplessOne1917 This feels like a relatively large decision, should we wait for @dessalines to weigh in?

@alectrocute
Copy link
Contributor

@alectrocute @SleeplessOne1917 This feels like a relatively large decision, should we wait for @dessalines to weigh in?

Yep, let’s do.

@jsit jsit added this to the 0.18.1 milestone Jun 26, 2023
@dessalines
Copy link
Member

dessalines commented Jun 26, 2023

I'm going to dissent and say I don't like it too much. What I discovered in playing around with this in jerboa, was that the icon size was actually more important in making things look cleaner: Right now, they're wayy too big in lemmy-ui. In jerboa they're properly sized to the text, and it makes a lot of difference.

post list-view:

Screenshot_2023-06-26-15-13-23-905_com jerboa

post card-view:

Screenshot_2023-06-26-15-16-49-006_com jerboa

And it smartly hides the community icon on the community page.

The other thing is that icons, even if they're tiny, really help with instant recognition of users and communities. When you have icons in one place but not the other, its throws that off.

@jsit
Copy link
Contributor Author

jsit commented Jun 26, 2023

@dessalines I hear you -- glad to have gotten your feedback on this before merging. Would it be a welcome PR to make them a little smaller in the post list view?

@alectrocute
Copy link
Contributor

@dessalines I hear you -- glad to have gotten your feedback on this before merging. Would it be a welcome PR to make them a little smaller in the post list view?

We should definitely at least make the avatars line-height IMO.

@dessalines
Copy link
Member

@jsit it'd be a good idea to make them match the text line height closely everywhere (except the tops of the profile and community pages).

@jsit
Copy link
Contributor Author

jsit commented Jun 26, 2023

@alectrocute @dessalines I've restored the icon size, and made the height/width equal to one line height. Updated screenshots in description.

@jsit jsit changed the title feat(UI): Hide avatars on listings feat(UI): Make post listing (icon) avatars smaller (line-height) Jun 26, 2023
Copy link
Member

@SleeplessOne1917 SleeplessOne1917 left a comment

Choose a reason for hiding this comment

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

I've restored the icon size, and made the height/width equal to one line height. Updated screenshots in description.

I think you might have borked a merge, because only code changes I see in the PR currently is passing hideAvatars throught post listings.

@jsit
Copy link
Contributor Author

jsit commented Jun 26, 2023

@SleeplessOne1917 Oops you're right, I forgot to push. Should be good now.

@alectrocute
Copy link
Contributor

Super excited for this PR. Thanks @jsit

@SleeplessOne1917 SleeplessOne1917 merged commit 531630c into LemmyNet:main Jun 27, 2023
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.

4 participants