Feature Request - Profile / Workspace - Allow avatar editing via cropping, rotation and zooming #8511
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
melvin-bot
bot
requested review from
parasharrajat and
tgolen
and removed request for
a team
| const rotation = useSharedValue(0); | ||
| const translateSlider = useSharedValue(0); | ||
|
|
||
| const containerSize = props.isSmallScreenWidth ? Math.min(props.windowWidth, 500) - 40 : variables.sideBarWidth - 40; |
There was a problem hiding this comment.
40 here is the padding. Can't use do something where it takes the whole width of the container without padding. And get the container Size via onLayout.
There was a problem hiding this comment.
Thank you for the idea. I'll do it.
| sliderLineWidth: PropTypes.number, | ||
|
|
||
| /** Callback to execute when user panning slider */ | ||
| onGestureEvent: PropTypes.func, |
There was a problem hiding this comment.
Can slider work without this method? It is linking the slider and Image modal together. The slider should work independently from the image modal and when the user makes changes to the slider, it should take a callback to pass the changed value to the image modal.
There was a problem hiding this comment.
Unfortunately, no. We have to have the ability to change the state of translateSlider value from the parent component.
| function getContext(canvas) { | ||
| return canvas.getContext('2d'); | ||
| } | ||
|
|
||
| function sizeFromAngle(width, height, angle) { | ||
| const radians = (angle * Math.PI) / 180; | ||
| let c = Math.cos(radians); | ||
| let s = Math.sin(radians); | ||
| if (s < 0) { | ||
| s = -s; | ||
| } | ||
| if (c < 0) { | ||
| c = -c; | ||
| } | ||
| return {width: (height * s) + (width * c), height: (height * c) + (width * s)}; | ||
| } | ||
|
|
||
| function rotate(canvas, degrees) { | ||
| const {width, height} = sizeFromAngle(canvas.width, canvas.height, degrees); | ||
|
|
||
| const result = document.createElement('canvas'); | ||
| result.width = width; | ||
| result.height = height; | ||
|
|
||
| const context = getContext(result); | ||
|
|
||
| // Set the origin to the center of the image | ||
| context.translate(result.width / 2, result.height / 2); | ||
|
|
||
| // Rotate the canvas around the origin | ||
| const radians = (degrees * Math.PI) / 180; | ||
| context.rotate(radians); | ||
|
|
||
| // Draw the image | ||
| context.drawImage(canvas, -canvas.width / 2, -canvas.height / 2, canvas.width, canvas.height); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| function crop(canvas, options) { | ||
| // ensure values are defined. | ||
| let { | ||
| originX = 0, originY = 0, width = 0, height = 0, | ||
| } = options; | ||
| const clamp = (value, max) => Math.max(0, Math.min(max, value)); | ||
|
|
||
| // lock within bounds. | ||
| width = clamp(width, canvas.width); | ||
| height = clamp(height, canvas.height); | ||
| originX = clamp(originX, canvas.width); | ||
| originY = clamp(originY, canvas.height); | ||
|
|
||
| // lock sum of crop. | ||
| width = Math.min(originX + width, canvas.width) - originX; | ||
| height = Math.min(originY + height, canvas.height) - originY; | ||
|
|
||
| const result = document.createElement('canvas'); | ||
| result.width = width; | ||
| result.height = height; | ||
|
|
||
| const context = getContext(result); | ||
| context.drawImage(canvas, originX, originY, width, height, 0, 0, width, height); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| function getResults(canvas) { | ||
| return new Promise((resolve) => { | ||
| canvas.toBlob((blob) => { | ||
| const file = new File([blob], 'fileName.jpg', {type: 'image/jpeg'}); | ||
| file.uri = URL.createObjectURL(file); | ||
| resolve(file); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| function loadImageAsync(uri) { | ||
| return new Promise((resolve, reject) => { | ||
| const imageSource = new Image(); | ||
| imageSource.crossOrigin = 'anonymous'; | ||
| const canvas = document.createElement('canvas'); | ||
| imageSource.onload = () => { | ||
| canvas.width = imageSource.naturalWidth; | ||
| canvas.height = imageSource.naturalHeight; | ||
|
|
||
| const context = getContext(canvas); | ||
| context.drawImage(imageSource, 0, 0, imageSource.naturalWidth, imageSource.naturalHeight); | ||
|
|
||
| resolve(canvas); | ||
| }; | ||
| imageSource.onerror = () => reject(canvas); | ||
| imageSource.src = uri; | ||
| }); | ||
| } |
There was a problem hiding this comment.
All of these need JSDoc. If the function name is sufficient then no need to add the comment.
There was a problem hiding this comment.
Added. Please check.
| onClose: PropTypes.func, | ||
|
|
||
| /** Callback to be called when user crops image */ | ||
| onCrop: PropTypes.func, |
There was a problem hiding this comment.
I think this should be named as onSave or onSubmit. If I am not wrong, this callback will be used when user submits the image.
There was a problem hiding this comment.
Renamed to onSave
| // eslint-disable-next-line no-param-reassign | ||
| context.translateX = translateX.value; | ||
| // eslint-disable-next-line no-param-reassign | ||
| context.translateY = translateY.value; |
There was a problem hiding this comment.
Is it necessary to modify context directly or can this method return a brand new context? (not sure how onStart() is meant to work)
There was a problem hiding this comment.
Unfortunately, yes. Reanimated 2 uses turbo modules, and that means that we have to change the same variable in memory.
| onActive: (event, context) => { | ||
| let heighRatio = 1.0; | ||
| let widthRation = 1.0; | ||
| if (imageWidth.value > imageHeight.value) { |
There was a problem hiding this comment.
Please add comments for why these ratios are being set to what they are.
There was a problem hiding this comment.
I've moved this logic to getDisplayedImageSize function and added a comment.
| widthRation = imageHeight.value / imageWidth.value; | ||
| } | ||
|
|
||
| const radius = containerSize / 2; |
There was a problem hiding this comment.
Please add a comment for why this value is what it is.
There was a problem hiding this comment.
Renamed radius to apothem and added a comment.
| const realImageHeight = radius * scale.value * widthRation; | ||
| const realImageWidth = radius * scale.value * heighRatio; | ||
|
|
||
| const maxX = realImageWidth - radius; |
There was a problem hiding this comment.
Please add comments to describe why these variables are necessary.
There was a problem hiding this comment.
I've moved this logic to updateImageOffset function and added a comment.
|
|
||
| const newX = event.translationX + context.translateX; | ||
| const newY = event.translationY + context.translateY; | ||
| translateX.value = interpolate(newX, [minX, maxX], [minX, maxX], 'clamp'); |
There was a problem hiding this comment.
Please add a comment to describe what the interpolate() method is for
There was a problem hiding this comment.
I've moved this logic to the clamp function and added a comment
| return result; | ||
| } | ||
|
|
||
| function getResults(canvas) { |
There was a problem hiding this comment.
This function name is too generic, can you please rename it to have a more specific meaning?
There was a problem hiding this comment.
Renamed to convertCanvasToFile
| @@ -107,6 +110,18 @@ class AvatarWithImagePicker extends React.Component { | |||
| this.setState({isMaxUploadSizeModalOpen: isVisible}); | |||
| } | |||
|
|
|||
| /** | |||
| * Checks if image has valid size and updates avatar | |||
There was a problem hiding this comment.
I'm not sure what "updates avatar" means in this context. Maybe this should just say that it triggers the prop callback?
There was a problem hiding this comment.
Changed to: "Checks if the image has the valid size and triggers the onImageSelected callback"
|
Comments are fixed and screen recordings are updated (+contributor checklist) |
There was a problem hiding this comment.
LGTM. Only thing is that I am not seeing a closing animation for the CropModal on the Web. I suspect the busy thread is causing that which is fine for me.
cc: @tgolen
PR Reviewer Checklist
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
- I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all
src/languages/*files - I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - Any functional components have the
displayNameproperty - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
🎀 👀 🎀 C+ reviewed
|
@tgolen we are creating a new canvas only when the image is saved. When we are pressing a rotate button, we just animate it. |
|
Last question, do we have an ability to add feature flags? We could easily add one for this feature, just in case. |
|
We do have the ability to add feature-specific betas, but I don't think we need one in this case. We don't have a whole lot of public users on the app yet, and I think the risk of having a bug here is very low (ie. it's not a catastrophic failure). It looks like the Jest Unit Tests are failing. It kind of looks like those are unrelated to your changes, so could you try updating your branch with |
|
Ready for merge. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| function convertCanvasToFile(canvas, options = {}) { | ||
| return new Promise((resolve) => { | ||
| canvas.toBlob((blob) => { | ||
| const file = new File([blob], `${options.name || 'fileName'}.jpg`, {type: 'image/jpeg'}); |
There was a problem hiding this comment.
How come we've got type: 'image/jpeg' here, while we have...
| RNImageManipulator.manipulate(uri, actions, options).then((result) => { | ||
| RNFetchBlob.fs.stat(result.uri.replace('file://', '')).then(({size}) => { | ||
| resolve({ | ||
| ...result, size, type: 'image/png', name: `${options.name || 'fileName'}.jpg`, |
There was a problem hiding this comment.
... type: 'image/png' here?
There was a problem hiding this comment.
I think this is a mistake. should be image/jpeg.
There was a problem hiding this comment.
Ya thought so - I don't believe it makes a difference at the moment - to solve #11063 we'll most likely try to make this type dynamic
|
Hey 👋. Looks like when we first implemented this modal we forgot to add tooltips for the rotate button and slider knob which resulted in this bug. |
|
We didn't think that tooltips were needed and they were not part of requirements. |
TODO:
Details
Fixed Issues
$ #6301
Tests
Verify that no errors appear in the JS console
Test image crop with gallery image
Preconditions: You have an image with a resolution lower than 80x80
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Verify that no errors appear in the JS console
Test image crop with gallery image
Screenshots
Web
Chrome.Screen.Recording.2022-08-03.at.22.22.18.mov
Safari.Screen.Recording.2022-08-03.at.22.21.05.mov
Mobile Web
Screen.Recording.2022-07-27.at.15.22.15.mov
Screen.Recording.2022-07-27.at.15.15.05.mov
Desktop
Desktop.Screen.Recording.2022-08-03.at.22.19.44.mov
iOS
Screen.Recording.2022-07-27.at.15.29.17.mov
Android
Screen.Recording.2022-07-27.at.15.32.07.mov