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

Fix/135 Re-adding "Square" Crop ability #136

Merged
merged 2 commits into from
Jun 22, 2022
Merged

Fix/135 Re-adding "Square" Crop ability #136

merged 2 commits into from
Jun 22, 2022

Conversation

faisal-alvi
Copy link
Member

@faisal-alvi faisal-alvi commented Jun 17, 2022

Description of the Change

Closes #135

Alternate Designs

n/a

Possible Drawbacks

Maybe in the future, we get some users wanting the freeform crop ability.

Verification Process

  1. Edit your profile.
  2. Set an avatar.
  3. See, you are forced to stick with the Square form while cropping an avatar.
  4. Also check if the button name from "Skip Crop" to "Default Crop" is only changed on the edit profile page, not any other page like when selecting the site icon in the customizer (wp-admin/customize.php?return=%2Fwp-admin%2Fprofile.php), the button name should be "Skip Crop".

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Added - Re-added the Square crop ability.

Credits

Props @dkotter @dinhtungdu @faisal-alvi

@faisal-alvi faisal-alvi added this to the 2.5.0 milestone Jun 17, 2022
@faisal-alvi faisal-alvi requested a review from a team June 17, 2022 11:54
@faisal-alvi faisal-alvi self-assigned this Jun 17, 2022
@faisal-alvi faisal-alvi requested review from peterwilsoncc and removed request for a team June 17, 2022 11:54
peterwilsoncc
peterwilsoncc previously approved these changes Jun 20, 2022
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

LGTM.

The skip cropping button remains in place (which actually uses the default square crop during testing), was that intentional? I am presuming it is and approving the change for now.

@faisal-alvi
Copy link
Member Author

faisal-alvi commented Jun 20, 2022

@peterwilsoncc Thanks for raising this point, this seems similar mentioned by @dinhtungdu in #131 (comment). Even if a user skips the crop, the end result will still be in the (default) square.

WordPress displays an avatar in its appropriate/nearest scaled size at FE, we are also using the same avatar size that is found in the arguments passed to get_avatar_data() and displaying our Simple Local Avatar (regardless of the avatar was skipped for crop or not).

$simple_local_avatar_url = $this->get_simple_local_avatar_url( $id_or_email, $args['size'] );

If a user crops avatar, a cropped image's scaled version is displayed at FE "vs", If cropping is skipped, the original image's scaled version is displayed. In either case, the scaled (or we can say "cropped") image is displayed at FE. This is WP's default behavior.

Now the question is - are we supposed to display an 'original' size avatar when the user clicks "Skip Cropping"? In that case, we have to add a new user meta that stores whether the avatar is cropped or not. And conditionally skip passing the size in the above code and use something like this:

$simple_local_avatar_url = $this->get_simple_local_avatar_url( $id_or_email, 0 ); (where 0 will bring the original image URL)

I have uploaded and set a 460 x 6358 (vertical) avatar image and used the 'original' version (by using the above code statically), and here are the results.

image

IMO, we should not change the WP's default behavior of displaying avatars at the front end (by displaying 'original' sized avatars) and keep using the "default" crop/scaled version of Avatar. However, I'm concerned users may get confused about why the avatar is in default cropped mode even if they have clicked "Skip Crop".

@dkotter @jeffpaul - please share your thoughts, is it fine to keep using the scaled avatar images by WordPress at the front end (even when the user chooses the "Skip cropping" while setting up an avatar in the backend?

@faisal-alvi faisal-alvi mentioned this pull request Jun 20, 2022
17 tasks
@peterwilsoncc
Copy link
Contributor

IMO, we should not change the WP's default behavior of displaying avatars at the front end (by displaying 'original' sized avatars) and keep using the "default" crop/scaled version of Avatar. However, I'm concerned users may get confused about why the avatar is in default cropped mode even if they have clicked "Skip Crop".

This makes sense.

How about instead of "Skip Crop" the text is changed to "Default Crop". This can be discussed on another ticket as this PR covers a different change.

@faisal-alvi
Copy link
Member Author

faisal-alvi commented Jun 22, 2022

@peterwilsoncc the "Skip Crop" is coming from the core and I didn't find a way to overwrite it.

https://github.com/WordPress/WordPress/blob/9e3dc52d45f6023df554ab421f9daf34f2383444/wp-includes/media.php#L4600

EDIT: Found the filter media_view_strings just below that code to modify the strings.

Copy link
Contributor

@peterwilsoncc peterwilsoncc 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, thanks.

@peterwilsoncc peterwilsoncc merged commit 9d66f67 into develop Jun 22, 2022
@peterwilsoncc peterwilsoncc deleted the fix/135 branch June 22, 2022 23:39
faisal-alvi added a commit that referenced this pull request Jun 23, 2022
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.

Re-add "Square" crop and remove "Freeform"
2 participants