Skip to content

Ahn nath/crop representatives photo: [Low Priority] Crop representative photos using Cicero API data#367

Merged
paramsiddharth merged 16 commits intomainfrom
ahn-nath/crop-representatives-photo
Oct 27, 2022
Merged

Ahn nath/crop representatives photo: [Low Priority] Crop representative photos using Cicero API data#367
paramsiddharth merged 16 commits intomainfrom
ahn-nath/crop-representatives-photo

Conversation

@ahn-nath
Copy link
Contributor

@ahn-nath ahn-nath commented Oct 23, 2022

Closes #333

In order to take advantage of the photo_cropping property that lets us improve the appearance of member/profile's pics via the API responses, we have:

  • Examined the photo_cropping property.
  • Planned out any modifications that need to be carried out before sending it to the front-end.
  • Applied inline styling to the photos being displayed in individual representative cards to crop them.
  • Documented and cleaned the code.

Approach:
The official documentation describes the photo_cropping property as such:

For photos that are zoomed out or off center, an object of the form {x, y, width, height, origWidth, origHeight} specifying the location (from top left) and dimensions of a bounding box that defines a headshot within the official photo.

It identifies or captures the location of the "ideal" headshot, and they provide the x, and y coordinates of the start of the object/headshot. An example of it:

"photo_cropping": { "x": 54, "y": 0, "width": 176, "height": 180, "origWidth": 450, "origHeight": 250 }

We can assume that representatives that do have this property (non-null) have a pic that is either zoomed out or off center. For that reason, instead of actually cropping the pictures and creating a function that makes that update, the code I added simply focuses on the frame or quadrant of the image where the object or headshot starts.

Consider the following:
image
The yellow ball, our object, is at the first quadrant, which means it is at the top-left side of our image of the bigger frame. We locate the object in accordance with its x-location (left or right) and its y-location (top or bottom) and then use the background-position property of the frontend component (v-img) to properly display the pic.

The logic we use is:
For the x-value or x-location:

- If the object/headshot starts at the first part of the left quadrant or side, then we assume the x-value to be left.
- If the object/headshot starts at the first part of the right quadrant or side, then we assume the x-value to be right.
- If none, we assume center as the x-value.

For the y-value or y-location:

- If the object/headshot starts at the first part of the top quadrant or side, then we assume the y-value to be top.
- If the object/headshot starts at the first part of the bottom quadrant or side, then we assume the y-value to be bottom.
- If none, we assume center as the y-value.

Then we simply concatenate both and use them as the CSS value of the background position property. We use the center of the image as a reference for the left and right sides, as well as the top and bottom sides.

@ahn-nath
Copy link
Contributor Author

Examples:
Each example will have the postal code, and the previous (from the production app) and after (the solution of current version of the changes I propose).

98101
Andrew J. Lewis

photo_cropping_wrongt_andrew_l

photo_cropping_correct_andrew_l

10001
Mark Levine
photo_cropping_wrong_mark_l

photo_cropping_correct_mark_l

97005
Tobias Read
photo_cropping_wrong_tobias_r

photo_cropping_correct_tobias_r

@ahn-nath
Copy link
Contributor Author

Exceptions:
If our app does not receive any photo_cropping object (null value), then we cannot modify the pic, even if the change is obvious for us. Consider Rebecca Rhynhart, postal code 19147. We should and can position the image to the right to have a better frame, but the photo_cropping object came as "null".

image
image

Nathaly Toledo and others added 9 commits October 24, 2022 19:25
@ahn-nath
Copy link
Contributor Author

@thyeggman

I appreciate your comments and the time you invested on this PR; they were helpful.

Copy link
Member

@paramsiddharth paramsiddharth left a comment

Choose a reason for hiding this comment

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

@ahn-nath Hi, Nath. I think the change in the package-lock.json is accidental and shouldn't be present here. Could you please revert it to the one in the main branch? You can see the changes here and realize that it mentions the axios-cache-interceptor. Maybe you installed it and then uninstalled it? Once you revert to the original package-lock.json, do an npm ci and it should fix everything. Let me know if you need my help.

@ahn-nath
Copy link
Contributor Author

@ahn-nath Hi, Nath. I think the change in the package-lock.json is accidental and shouldn't be present here. Could you please revert it to the one in the main branch? You can see the changes here and realize that it mentions the axios-cache-interceptor. Maybe you installed it and then uninstalled it? Once you revert to the original package-lock.json, do an npm ci and it should fix everything. Let me know if you need my help.

I will update it soon and commit the fix. Thanks!

Copy link
Contributor Author

@ahn-nath ahn-nath left a comment

Choose a reason for hiding this comment

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

@paramsiddharth
I have reviewed the relevant files with a git checkout origin/main package-lock.json command.

thyeggman
thyeggman previously approved these changes Oct 25, 2022
paramsiddharth
paramsiddharth previously approved these changes Oct 26, 2022
Copy link
Member

@paramsiddharth paramsiddharth left a comment

Choose a reason for hiding this comment

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

@ahn-nath Good job, as always, Nath. Just tested it out locally. It works amazing! I appreciate how you described the design of getPhotoCroppingValues in the PR description. The function is really well-written and functioning. We don't need to worry about the null values — If the people at Cicero weren't able to figure those out for us, we shouldn't be concerned either, haha!

Despite it being a low-priority issue, you've done a good job with the research and quality, once again. Let's wait on Sam's PR (#366) to be merged to minimize the conflicts. Meanwhile, you can work on implementing #294, because you've already done the research and we have a clear idea that we're moving forward with the cache interceptor library. Keep it up!

@sammychinedu2ky
Copy link
Contributor

Hi @ahn-nath , the logic here is pretty cool 😅

@paramsiddharth paramsiddharth merged commit 2578f65 into main Oct 27, 2022
@paramsiddharth paramsiddharth deleted the ahn-nath/crop-representatives-photo branch October 27, 2022 13:11
@joeyouss joeyouss added week 5 and removed week 1 labels Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

[front-end] Crop representative photos using Cicero API data

6 participants