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 Avatar Border for Visual Consistency #32574

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

kerwin612
Copy link
Member

For the ① mentioned in the comment (#32565 (comment)).

Add a border to the Avatar to avoid poor visual experiences caused by irregularly sized images.

before:
image

after:
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 20, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 20, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Nov 20, 2024
@lunny
Copy link
Member

lunny commented Nov 20, 2024

The new design looks a bit off—there are two overlapping borders.

@wxiaoguang
Copy link
Contributor

To be honest, the border is more ugly than before

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 20, 2024
@kerwin612
Copy link
Member Author

To be honest, the border is more ugly than before

This is indeed a matter of "choice". No matter how one chooses, there will always be some who find it suitable and some who don't. This is quite normal, and I fully understand it.

Currently:
1732090781438

After adding the border:
1732090730917
(issues such as border style can be adjusted additionally)

I think we should first make a choice and confirm whether we need borders.
If it's confirmed that borders are needed, I will continue to improve this PR. If not, I will close this PR.

@lunny @wxiaoguang

@github-actions github-actions bot removed the modifies/go Pull requests that update Go code label Nov 20, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2024

I think we should first make a choice and confirm whether we need borders. If it's confirmed that borders are needed, I will continue to improve this PR. If not, I will close this PR.

I would say no to current state, these screenshots look more ugly.

image

image

@lunny

This comment was marked as outdated.

@silverwind
Copy link
Member

silverwind commented Nov 20, 2024

I personally dislike both circling and border, with circling being the more severe of the two. If there is a border, there should also be a subtle background change. And it wouldn't fit in all places, like on the user page where it looks strange imho with the double border.

When GitHub introduced the circles around 2 years ago, they visually broke a lot of avatars where essential parts of the avatar were cut off. So it's a somewhat breaking change too.

@kerwin612
Copy link
Member Author

Can we initiate a vote to decide whether to adopt borders or not?
Known: Gitea's avatars can be edited through API and there is no aspect ratio restriction; therefore, there must exist issues of visual inconsistency as described in issue #31990.

Option 1: Keep the current status;
Option 2: Add a simply border; (current PR)
Option 3: Add a circular border.
Option 4: Other ...

@lunny
Copy link
Member

lunny commented Nov 25, 2024

Can we initiate a vote to decide whether to adopt borders or not? Known: Gitea's avatars can be edited through API and there is no aspect ratio restriction; therefore, there must exist issues of visual inconsistency as described in issue #31990.

Option 1: Keep the current status; Option 2: Add a simply border; (current PR) Option 3: Add a circular border. Option 4: Other ...

I prefer Option 2 but with no double borders now.

@kerwin612
Copy link
Member Author

(25f2062) The border and background color of the cards on the left side of the profile have been removed, avoiding the "double border" issue that arises when adding a border to the avatar.
5535b3f24d582918de52616e4805fcc

@kerwin612 kerwin612 requested a review from lunny November 27, 2024 00:32
lunny
lunny previously approved these changes Nov 27, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 27, 2024
@wxiaoguang
Copy link
Contributor

Does it look right here? None of these icons has border except the avatar?

image

@kerwin612
Copy link
Member Author

Does it look right here? None of these icons has border except the avatar?

image

Simply adding borders to icons is too ugly:
a52ede6addd9e456248e292b6329bfe

Refactor the layout to make it look like it's coordinated:
b1c0cb335eaefbb282379d082825f90

But it also involves mobile adaptation, which is a bit complicated, so I'll try again.

@wxiaoguang wxiaoguang marked this pull request as draft November 28, 2024 08:46
@wxiaoguang
Copy link
Contributor

But it also involves mobile adaptation, which is a bit complicated, so I'll try again.

Yup, the improvements need to be designed globally and carefully.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 28, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Nov 28, 2024
@kerwin612
Copy link
Member Author

before:
e34ac18f098a4878bee86a93eef170c
after:
215ff1bada5aff5c03c356a3f03eb3f

before:
126af24c70a8a1b70ffd6f9cb71db48
after:
beb82d692bb809746c3c8f206a21ba3

Avoiding excessively broad and significant changes within a single pull request

@kerwin612 kerwin612 marked this pull request as ready for review November 28, 2024 14:37
@lunny
Copy link
Member

lunny commented Nov 28, 2024

I think we should add the avatars border in this pull request but not other buttons. But the new look of profile page is not good to me.

@lunny lunny dismissed their stale review November 28, 2024 18:21

code change

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Nov 28, 2024
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 29, 2024
@github-actions github-actions bot removed the modifies/templates This PR modifies the template files label Nov 29, 2024
@kerwin612 kerwin612 requested a review from lunny November 29, 2024 00:28
@kerwin612
Copy link
Member Author

Then this PR returns to its original theme, solely addressing the visual experience issue caused by avatars of different proportions by adding a unified and proportional border to them.

Any subsequent optimizations or suggestions arising from this will be handled in separate PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants