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

Add small-user-avatars feature #6492

Merged
merged 8 commits into from
Apr 12, 2023
Merged

Add small-user-avatars feature #6492

merged 8 commits into from
Apr 12, 2023

Conversation

kidonng
Copy link
Member

@kidonng kidonng commented Apr 10, 2023

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Looks good except a couple small issues

source/features/small-user-avatars.tsx Outdated Show resolved Hide resolved
source/features/small-user-avatars.tsx Outdated Show resolved Hide resolved
@fregante
Copy link
Member

That looks better, but the alignment is off, see how nicely the name was wrapped before and how it's off-center now:

fixed

Also the height and awaitDomReady: true is causing content shift:

fixed

Maybe we should reduce the avatar and padding around it


/* Extra padding for `small-user-avatars` feature */
.rgh-collaborator:has(.avatar) {
padding-bottom: 2px;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think any of this CSS should be used, it's causing the misalignment and layout shift. It's best to align the image directly

Copy link
Member

@fregante fregante Apr 10, 2023

Choose a reason for hiding this comment

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

Here's a zero-shift image that can be inlined there without this CSS (rough values, just show it's possible)

<img class="from-avatar avatar-user" src="https://avatars.githubusercontent.com/u/44045911?s=40&amp;v=4" width="14" height="14" alt="@kidonng" style="
    margin-right: 3px;
    vertical-align: -2px;
    margin-left: -2.6px;
">

fixed

Tested in Chrome only, may need inline-flex or em to avoid font weirdness

Copy link
Member Author

Choose a reason for hiding this comment

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

I really want to stick to 16px as that's what we use for reaction-avatars, but yeah 14px is fine, this place is just too small.

This is how it looks like now, without any magic numbers.

image

Copy link
Member

@fregante fregante Apr 10, 2023

Choose a reason for hiding this comment

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

Let's not bring these PRs into loops. I already commented on that, it's not aligned and it should be fixed. At a minimum it needs some negative left-margin. Maybe .ml-n1 if it exists

Copy link
Member Author

@kidonng kidonng Apr 10, 2023

Choose a reason for hiding this comment

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

This is the best I can do:

image

Perhaps I'm really bad at CSS, I just don't know how to

  • Not change highlight-collaborators-and-own-conversations
  • Not causing content shifts
  • And have all these places aligned

image

So I don't get which places you want it aligned.

Did you mean the right side does not need to be aligned in #6492 (comment)? I certainly did not get that until now.

Copy link
Member

Choose a reason for hiding this comment

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

I mean I want it as it appears in the gif I provided and the magic-numbered CSS. It's ok, I can look into a proper solution later if there's no easy way to achieve it. Dealing with text alignment is always a pain in CSS

@kidonng kidonng requested a review from fregante April 11, 2023 15:58
Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

fixed

Looking good!

@fregante fregante merged commit bac8aec into main Apr 12, 2023
9 checks passed
@fregante fregante deleted the small-user-avatars branch April 12, 2023 09:04
@fregante fregante restored the small-user-avatars branch April 12, 2023 09:05
@fregante fregante deleted the small-user-avatars branch April 12, 2023 09:19
@kidonng
Copy link
Member Author

kidonng commented Apr 12, 2023

Oops, this would be better using selector-observer right?

EDIT: done in #6488

@fregante
Copy link
Member

Judging by the behavior on load, it doesn't need to: GitHub's own relative-time handler runs on domready apparently.

It's ok either way, the "deduplicate" logic is essentially deprecated now because we can't really trust it, it can change at any time and break everything again.

@@ -162,6 +162,7 @@ Thanks for contributing! 🦋🙌
- [](# "clean-repo-tabs") [Moves the "Security" and "Insights" to the repository navigation dropdown. Also moves the "Projects", "Actions" and "Wiki" tabs if they're empty/unused.](https://user-images.githubusercontent.com/16872793/124681343-4a6c3c00-de96-11eb-9055-a8fc551e6eb8.png)
- [](# "repo-avatars") [Adds the profile picture to the header of public repositories.](https://user-images.githubusercontent.com/44045911/177211845-c9b0fa37-c157-4449-890e-af2602c312e3.png)
- [](# "quick-new-issue") [Adds a link to create issues from anywhere in a repository.](https://user-images.githubusercontent.com/1402241/218251057-b94b62dd-a944-4763-b78a-fc233f7c9fd3.png)
- [](# "small-user-avatars") [Shows a small avatar next to the username in conversation lists.](https://user-images.githubusercontent.com/44045911/230960291-721f42cc-e1ac-4fdc-83ea-2430b062f9ce.png)
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this in your lint PR? All screenshots should be in light mode and with an arrow ideally. Also it's best to disable nearby features to avoid confusion

Screenshot 6

@Vendicated
Copy link

I love this, thank you!!

@fregante
Copy link
Member

Misaligned on iOS, this is why I was suggesting flexbox 🥲

IMG_7612

@fregante
Copy link
Member

Also it makes the list a little heavy/busy 🥲 it's probably worth it though, let's see how the feature is received.

Before

Screenshot 3

After

Screenshot 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add small-user-avatars feature
3 participants