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

feat(CropImageTool): Crop profile picture tool #1253

Merged
merged 36 commits into from
Oct 10, 2023

Conversation

lgmarchi
Copy link
Contributor

What this PR does 📖

1. Add possibility to crop image for profile picture

Screen.Recording.2023-09-28.at.14.35.15.mov

Which issue(s) this PR fixes 🔨

Special notes for reviewers 🗒️

Additional comments 🎤

@lgmarchi lgmarchi self-assigned this Sep 28, 2023
@github-actions github-actions bot added Don't merge yet DO NOT MERGE Missing dev review Still needs to be reviewed by a dev labels Sep 28, 2023
@phillsatellite
Copy link
Contributor

Functionally works awesome! I wanted to comment that I think its a little hard to see the slider when in light mode but that could just be my bad eyes lol other than that awesome feature so far 🔥

Screenshot_2023-09-28_at_2 58 23_PM

@phillsatellite phillsatellite added the Changes requested When other dev or QA request a change label Sep 28, 2023
@github-actions github-actions bot added the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Sep 28, 2023
@lgmarchi
Copy link
Contributor Author

Functionally works awesome! I wanted to comment that I think its a little hard to see the slider when in light mode but that could just be my bad eyes lol other than that awesome feature so far 🔥

Screenshot_2023-09-28_at_2 58 23_PM

You are correct, in dark mode is not perfect yet, but UI improvement I will let for next PR my friend.

@lgmarchi lgmarchi added Ready for testing Ready for QA to test and removed Changes requested When other dev or QA request a change labels Sep 28, 2023
@phillsatellite phillsatellite removed the Ready for testing Ready for QA to test label Sep 28, 2023
@lgmarchi
Copy link
Contributor Author

lgmarchi commented Oct 5, 2023

Testing under linux (using opensuse but i assume it is applicable on other distros) that when you select an image and zoom in with the crop tool that the image is partial and may disappear when there is any update to the UI.

2023-10-04.23-08-01.mp4

Did you have similar problem on Linux @phillsatellite ?

@phillsatellite
Copy link
Contributor

When testing last night I couldnt replicate, I re-tested again with Darius and finally got the bug to appear

On Linux I set my profile picture to abour 3/4th of the way zoomed in and when I set picture it just appeared blank. It took me about 15-20 tries to try to replicate this, it seemed like maybe it was at that certain spot when zooming in that caused the bug to appear
Screenshot_2023-10-06_at_12 15 26_PM

@phillsatellite phillsatellite added Changes requested When other dev or QA request a change and removed QA Tested QA has tested and approved labels Oct 6, 2023
@phillsatellite
Copy link
Contributor

Was talking with Sara and Lucas this morning were thinking a better route is to get this merged in and create a new ticket for the Linux bug 👍

@phillsatellite phillsatellite added QA Tested QA has tested and approved and removed Changes requested When other dev or QA request a change Failed Automated Test This PR is failing Luis's Appium test and needs revised labels Oct 10, 2023
Copy link
Contributor

@sdwoodbury sdwoodbury left a comment

Choose a reason for hiding this comment

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

it looks functional but before approving I have some suggested improvements.

edit: the following got left out of the review - the eval on line 49, which uses the first_render hook - could be executed inside of an onmounted event of the div that starts on line 81.

ui/src/components/crop_image_tool/mod.rs Show resolved Hide resolved
ui/src/components/crop_image_tool/mod.rs Show resolved Hide resolved
@sdwoodbury sdwoodbury added Changes requested When other dev or QA request a change and removed Missing dev review Still needs to be reviewed by a dev labels Oct 10, 2023
@lgmarchi
Copy link
Contributor Author

it looks functional but before approving I have some suggested improvements.

edit: the following got left out of the review - the eval on line 49, which uses the first_render hook - could be executed inside of an onmounted event of the div that starts on line 81.

I inserted inside use_future, it will run just one time as well.

@lgmarchi lgmarchi removed the Changes requested When other dev or QA request a change label Oct 10, 2023
@stavares843 stavares843 added Waiting for CI Waiting for at least one CI job to complete before merging and removed QA Tested QA has tested and approved Don't merge yet DO NOT MERGE labels Oct 10, 2023
@stavares843 stavares843 merged commit a17d660 into dev Oct 10, 2023
@stavares843 stavares843 deleted the crop-profile-picture-tool branch October 10, 2023 17:38
@github-actions github-actions bot added Failed Automated Test This PR is failing Luis's Appium test and needs revised and removed Waiting for CI Waiting for at least one CI job to complete before merging labels Oct 10, 2023
@stavares843 stavares843 removed the Failed Automated Test This PR is failing Luis's Appium test and needs revised label Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants