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.

Sign up for GitHub

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

Jump to bottom

raising profile photo size to comply with frio sidebar profile photo scaling #6009

Merged
merged 3 commits into from Oct 23, 2018
Merged

raising profile photo size to comply with frio sidebar profile photo scaling #6009

merged 3 commits into from Oct 23, 2018

Conversation

vinzv
Copy link

@vinzv vinzv commented Oct 23, 2018

fixing #5992

MrPetovan
MrPetovan requested changes Oct 23, 2018
View reviewed changes
Copy link
Collaborator

@MrPetovan MrPetovan left a comment

Choose a reason for hiding this comment

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

Please change everywhere else we use 175 as a photo size, including in the addons and the default avatar image.

@vinzv
Copy link
Author

vinzv commented Oct 23, 2018

I changed the default avatar and added a 300px version of it, if that's what you meant.

For addons I don't think there are changes needed as my initial commit is only for the automated resizing of uploaded profile pictures.

@MrPetovan
Copy link
Collaborator

I understand, but we have different hard-coded images scales:

  • micro: 48px max
  • thumb: 80px max
  • small: 175px max
  • medium: 600px max
  • large: 1024px max

In mod/proxy.php for example, small images will be scaled down to 175px, in addon/catavatar/catavatar.php, avatars will be created at 175px, in include/api.php, profile images bigger than 175 pixels will be scaled down as well.

I don't have a problem with raising the small size from 175 to 300 pixels, but it has to be done consistently through the whole project, addons included.

annando
annando reviewed Oct 23, 2018
View reviewed changes
mod/dfrn_confirm.php
@@ -537,7 +537,7 @@ function dfrn_confirm_post(App $a, $handsfree = null)
if (DBA::isResult($contact)) {
$photo = $contact['photo'];
} else {
$photo = System::baseUrl() . '/images/person-175.jpg';
$photo = System::baseUrl() . '/images/person-300.jpg';
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no "person-300.jpg" on our system.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The PR actually creates one.

Copy link
Author

Choose a reason for hiding this comment

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

I added it.

@annando
Copy link
Collaborator

annando commented Oct 23, 2018

We have several image sizes for the contact. I'm unsure which size the biggest one does have. Possibly this already helps.

@MrPetovan
Copy link
Collaborator

@annando Please refer to the image scale list I posted earlier.

@vinzv vinzv mentioned this pull request Oct 23, 2018
Merged
@vinzv
Copy link
Author

vinzv commented Oct 23, 2018

So I went to the whole code and changed 175 to 300 where it seemed appropriate.

MrPetovan
MrPetovan approved these changes Oct 23, 2018
View reviewed changes
Copy link
Collaborator

@MrPetovan MrPetovan left a comment

Choose a reason for hiding this comment

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

Thanks for your work, I spotted references to 175px in different theme templates but we'll let browsers take care of the downscaling.

@MrPetovan MrPetovan added this to the 2018.12 milestone Oct 23, 2018
@MrPetovan MrPetovan merged commit 1a1300f into friendica:develop Oct 23, 2018
MrPetovan
MrPetovan reviewed Oct 23, 2018
View reviewed changes
mod/profile_photo.php
@@ -288,7 +288,7 @@ function profile_photo_crop_ui_head(App $a, Image $image)
$height = $image->getHeight();

if ($width < 175 || $height < 175) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realized that those values probably need to be raised to 300 as well.

Copy link
Author

Choose a reason for hiding this comment

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

Don't think so, as this is for upscaling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are probably right, let's see how this pans out.

@vinzv
Copy link
Author

vinzv commented Oct 23, 2018

Yeah, I didn't touch the themes to avoid any breakage. Browser's downscaling should be fine.

Repository owner deleted a comment from codecov-io Oct 23, 2018
@vinzv vinzv mentioned this pull request Oct 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annando annando annando left review comments

@MrPetovan MrPetovan MrPetovan approved these changes

Assignees
No one assigned
Labels
Enhancement UX
Projects
None yet
Milestone
2019.01
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vinzv @MrPetovan @annando