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

post id avatars #1374

Merged
merged 4 commits into from
Feb 5, 2025
Merged

post id avatars #1374

merged 4 commits into from
Feb 5, 2025

Conversation

Mjokfox
Copy link
Contributor

@Mjokfox Mjokfox commented Dec 31, 2024

Wasn't a fan of 'gravatar', an external way to give users avatars, knowing e621 has the ability to set a post as your avatar, i wanted the same, and others might like it too so hence a pr.

since it uses user_config, it has to get the user config every time for the owner of every comment, using user_config::get_for_user($id) seemed slow as it sends a quite large event every time, so it directly gets the config in User::get_avatar_post_link(), but i am not entirely sure if its safe to do it in this way. The url or full html might also be cacheable to increase efficiency a bit thinking about it now.

@discomrade
Copy link
Contributor

Hi @Mjokfox, it looks like the formatting test didn't pass. The main branch is locked down so it can't accept pull requests that don't pass the tests.

https://github.com/shish/shimmie2/actions/runs/12563091233/job/35024471162?pr=1374

The tests can be run locally, see CONTRIBUTING.md for examples.

I'd like to see this added, already looks great with those foxes.

@Mjokfox
Copy link
Contributor Author

Mjokfox commented Feb 4, 2025

Thanks for the feedback! Not familiar yet with such checks, fixed it now :3

Copy link
Owner

@shish shish left a comment

Choose a reason for hiding this comment

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

A couple of very minor nits but yes this looks good to me :)

ext/user/main.php Outdated Show resolved Hide resolved
core/user.php Outdated Show resolved Hide resolved
@shish
Copy link
Owner

shish commented Feb 4, 2025

(Also apologies for the delay with getting to this, I've been on a yak-shaving adventure, trying to resuscitate an abandoned library that shimmie depends upon >.>;; )

@Mjokfox
Copy link
Contributor Author

Mjokfox commented Feb 4, 2025

Alright, added your suggestions.

I do still have the idea of adding a way to crop and resize the avatars using CSS, so the actual image does not need to change, while giving the user the ability to have their avatar be only part of an image. Basically store a "zoom" and "center position" in the db. I haven't tried any of that yet but I'd be willing to try and make it work if that is of any interest here as well?

@shish
Copy link
Owner

shish commented Feb 5, 2025

What's here now looks like a useful and stable milestone so I'mma merge as-is~

(Adding cropping sounds like it'd be nice - though I wouldn't count myself as "interested" per se, since the galleries I'm involved with already have a lot of user-investment in the gravatar approach and users are resistant to change 😅 But yeah it seems like a good thing for new installs to use)

@shish shish merged commit a91069e into shish:main Feb 5, 2025
16 checks passed
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.

3 participants