search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.59k stars 2.92k forks source link

[$8000] Feature Request - Profile / Workspace - Allow avatar editing via cropping, rotation and zooming - Reported by: @parasharrajat #6301

Closed isagoico closed 2 years ago

isagoico commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Navigate to profile settings
  2. Upload a new profile picture

Expected Result:

User should be able to edit the picture before setting the new avatar (Like zooming and cropping)

Actual Result:

User is not given the option to edit the profile picture when uploading a new avatar.

Workaround:

User has to edit the picture before uplaoding.

Platform:

Where is this issue occurring?

Version Number: 1.1.14-0

Reproducible in staging?: Yes Reproducible in production?: Yes

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: @parasharrajat Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1636994572206400

View all open jobs on GitHub

Solution

This is the final UI agreed upon: image

The two new icons you will need (zoom, rotate) are here: Icons.zip

mdneyazahmad commented 2 years ago

Agree with @Tabakharniuk proposal. If I am not wrong, we also have to setup expo packages.

For bare React Native projects, you must ensure that you have installed and configured the expo package before continuing.

mananjadhav commented 2 years ago

@parasharrajat Are we open for third party libraries and especially Expo ? If we are allowing expo then I feel https://github.com/Expensify/App/issues/6301#issuecomment-1014639018 is better ?

parasharrajat commented 2 years ago

I am ok with only image manipulation (cropping) but not with using a third-party lib to manage the Transformation. I think cropping will require some native solutions. But if we can do that via custom code that would also be great.

In short, a function or lib that takes the image and coordinates for the area and crops the image to that dimensions.

mdneyazahmad commented 2 years ago

@parasharrajat My solution is exactly what you are recommending. We will be using any third party library just to manipulate the image. If you go with expo-image-manipulator. We also need to setup our repository with expo managed project.

parasharrajat commented 2 years ago

Could we please wait for technical details? I am happy to hear all the proposals whatever they are. But yeah expo-image-manipulator has more features than we need.

But let me clear this internally.

mdneyazahmad commented 2 years ago

I have 1 more question. Can we use react-native-guesture-handler and react-native-reanimated. We already have it installed in our project?

parasharrajat commented 2 years ago

I think yes.

vitalii-tb commented 2 years ago

How will implement this in our app? It is important that it works on all platforms that we support.

I am planning to create new files inside App/src/components/AvatarCropModalwith a names AvatarCropModal.js, components/RotateButton.js, components/Slider.js and components/ImageCropView.js.

Inside index.js I will write functions using reanimated API to limit/validate zoom, pan gesture And to calculate cropping area. And pass them to Slider and ImageCropView components.
AvatarCropModal.js

                    <ImageCropView
                      value={imagePosition}
                      onChange={handleImagePostionChange}
                    />
                    <View style={styles.flexRow} >
                         <Slider
                          value={sliderValue}
                          onChange={handleSliderChange}
                          /> 
                          <RotateButton
                          onPress={handleRotate}
                          />
                    </View>
                     <Button
                            success
                            onPress={handleCrop}
                            style={styles.w100}
                            text={this.props.translate('common.save')}
                            pressOnEnter
                        />

The component will have api like this:

                    <AvatarCropModal
                        isVisible={isAvatarCropVisible}
                        uri={imageUri}
                        onConfirm={(newImageUri) => {
                                  PersonalDetails.setAvatar(newImageUri)
                                  setAvatarCropVisible(false)
                        }}
                    />

All code will be fully cross platform (iOS/android/electron/web).

What will be the flow for a feature?

Where will you hook this in our app? I think there are two places mostly that require avatar uploading.

I plan to use AvatarCropModal inside AvatarWithImagePicker.js, in that case, we will have it in those two places.

What are the hidden challenges?

Do you have questions for us?

@parasharrajat does that answer your questions?

mananjadhav commented 2 years ago

Proposal

Summary

Offline support

The user will be allowed to perform all editing actions. In case the user is offline and wishes to store the local copy of the cropped image and then upload it when the device comes online is not added in the current scope. This isn’t supported in the current implementation too.

Detailed background

Detailed implementation of the solution

UI Component

Zoom & Drag

Rotate & Crop

API Call

Manual tests

Automated tests

Alternate solutions (detailed)

System requirements

Plan of action / Rollout plan

Questions:

vitalii-tb commented 2 years ago

If you go with expo-image-manipulator. We also need to setup our repository with expo managed project.

But yeah expo-image-manipulator has more features than we need.

@parasharrajat No problems here. As I mentioned above:

If devs from Expensify will be against adding expo-image-manipulator & expo we could use @react-native-community/image-editor, and use browser API to crop image on web/electron

anthony-hull commented 2 years ago

What about using an implementation of Canvas on native? https://www.npmjs.com/package/react-native-canvas Then you could do all the image manipulation there on all platforms and save it out of the canvas. Might be some performance hits on it being all implemented in JS though.

mananjadhav commented 2 years ago

What about using an implementation of Canvas on native?

I've used react-native-canvas earlier and it is generally not maintained very well + a lot buggy.

anthony-hull commented 2 years ago

Good to know. Probably not the best idea then!

parasharrajat commented 2 years ago

Thanks for your proposals. I will review them as soon as possible. It is a big feature so I don't want to miss important steps and clarify as much as possible. I am sure that everyone has put a lot of effort into this (including me I had also created a demo component from existing functionality but not posted here). I would request you to be patient during the review process.

MelvinBot commented 2 years ago

Triggered auto assignment to @SofiedeVreese (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

mallenexpensify commented 2 years ago

@SofiedeVreese I'm out next week so auto-added you to keep 👀 on this til I'm back, then I'll take back over.

SofiedeVreese commented 2 years ago

Not overdue Melvin, this is a huge feature and we're taking our time to review this.

parasharrajat commented 2 years ago

I am going to handle this, this week. Proposals look promising.

SofiedeVreese commented 2 years ago

Hey @parasharrajat just wanting to follow up on this one? Thank you!

parasharrajat commented 2 years ago

Thanks @SofiedeVreese. I was busy some stuff. I will review the proposals soon. ETA - 2 days.

MelvinBot commented 2 years ago

@tgolen, @mallenexpensify, @parasharrajat, @SofiedeVreese Whoops! This issue is 2 days overdue. Let's get this updated quick!

parasharrajat commented 2 years ago

Coming back to this by tomorrow.

vitalii-tb commented 2 years ago

@parasharrajat any updates on this issue?

parasharrajat commented 2 years ago

Yeah, I am coming back to it. Give me a few hours to analyze. Although, there are not many questions I just want to be sure we are not missing steps.

At this stage, you can help with this one?

  1. How are you taking the dimensions of the canvas area?
  2. What if the uploaded image is smaller than that canvas area?

@Tabakharniuk You mentioned using custom logic which is the desired requirement and videos look promising.

@mananjadhav I see that you mentioned using libs for transformations. I think I agree with Tim's comment to use custom logic. Do you have any comments about it?

@mdneyazahmad Unfortunately, you have a similar proposal to extend a third part lib by forking is worse than using third-party lib.

I will go over each asked question and try to answer.

mananjadhav commented 2 years ago

About third party libraries, the packages that I’ve mentioned are maintained by react-native-community. For the native platforms I strongly feel we should use.

For the web components, the library that I mentioned, we can extract the transformation functions that work on Canvas.

parasharrajat commented 2 years ago

While you are answering these questions @Tabakharniuk . I think your proposal more closely matches the requirements. But I think it may be missing some technical details. @tgolen Do you have any questions regarding the proposal https://github.com/Expensify/App/issues/6301#issuecomment-1032480186 or are you looking for some specific details in the proposal?

parasharrajat commented 2 years ago

cc: @tgolen :arrow_up:

tgolen commented 2 years ago

with a names index.js

index.js is only used when we do platform specific files, and this is not a platform specific file, so please give it the same name as the component it exports.

I will write functions using reanimated api to limit/validate zoom, pan gesture And to calculate cropping area

Can you please include psuedo code for these methods?

onChange={handleSliderChange}

Please remember that methods should be named for what they do, not for what they handle (speaking about handleSliderChange()

All code will be fully cross platform (iOS/android/electron/web).

Will this happen out-of-the-box or is there platform specific stuff that will be normalized as part of this proposal?

we could use @react-native-community/image-editor, and use browser API to crop image on web/electron

Do we need image-editor? How much of this can be written without any external libs?

Which image formats should we support?

Which ones do you suggest? I would say, let's at least support the most standard ones.

What is the max resolution of the image that may be sent to the server?

I don't know that resolution matters, but maybe the size does? I think we can handle pretty big uploads, but I can find the max size once we need it.

In which format image should be sent to the server?

I would look to see whatever formats we currently support for displaying an avatar. I don't think it matters a whole lot what format we store it in as long as it can be displayed without having to be converted before display (ie. if any conversion is necessary, then it should be done before storage).

Hope that helps!

tgolen commented 2 years ago

components/RotateButton.js, components/Slider.js and components/ImageCropView.js

Please include psuedo code for these.

parasharrajat commented 2 years ago

Thanks, tgolen for your review. @Tabakharniuk, questions & requests :arrow_up: .

vitalii-tb commented 2 years ago

Thank you @parasharrajat & @tgolen for you rewiews!

Answers to @parasharrajat questions:

How are you taking the dimensions of the canvas area? Since there are no designs, I've roughly estimated that canvas takes 90% of the screen on mobile. And in my opinion, 400 is is a good match on desktop. Pseudocode:

const LARGE_SCREEN_CANVAS_SIZE = 400;
const size = isSmallScreenWidth ? windowWidth * 0.9 : LARGE_SCREEN_CANVAS_SIZE;

What if the uploaded image is smaller than that canvas area?

The component supports images with a smaller size than canvas. It will just scale contain the canvas.

Answers to @tgolen questions:

index.js is only used when we do platform specific files, and this is not a platform specific file, so please give it the same name as the component it exports.

Okay, I updated the name in the initial proposal to AvatarCropModal.js

I will write functions using reanimated API to limit/validate zoom, pan gesture And to calculate cropping area Can you please include pseudo code for these methods?

That is general js pseudocode.

const translateY = useSharedValue(0);
const translateX = useSharedValue(0);
const scale = useSharedValue(1);
const rotation = useSharedValue(0);
const translateSlider = useSharedValue(0);

function handlePan(event){
    translaveY = validateY(translaveY + event.translateY)
    translaveX = validateX(translaveX + event.translateX)
}

function handleZoom(event){
    scale +=  event.translateX
}

function handleRotate(){
    rotation =+ 90;
}

function clopImage(){
    const crop = {
      height: size,
      width: size,
      originX:
        centerX -
        radius -
        (translateX.value / scale.value),
      originY:
        centerY -
        radius -
        (translateY.value / scale.value),
    };

    const result = await ImageManipulator.manipulateAsync(
      imageUri,
      [
        {
          crop,
        },
      ],
    );
}

Will this happen out-of-the-box or is there platform specific stuff that will be normalized as part of this proposal?

Pan/Zoom/Rotate everything will work out of the box for ios/android/web/desktop.

Do we need image-editor? How much of this can be written without any external libs?

Yes, we definitely need it. I really would not recommend writing any native cropper code for ios/android, since there is a lot of different image format, and the risk of having bugs is high & a high support cost. In any case, we would need a library to crop on ios and another to crop on android. image-editor works fine on both platforms out of the box and is battle-tested. Without any libs, we could write only for the web.

Which image formats should we support? Which ones do you suggest?

image/jpg image/jpeg image/png image/bmp image/heic

components/RotateButton.js, components/Slider.js and components/ImageCropView.js Please include psuedo code for these.

components/Slider.js

   <View style={styles.sliderLine}>
          <PanGestureHandler onGestureEvent={handleZoom}>
            <AnimatedView style={styles.sliderKnob} />
          </PanGestureHandler>
    </View>

components/RotateButton.js

   <TouchableOpacity onPress={handleRotate} style={styles.rotateButton}>
    <Icon src={Expensicons.rotate} 
   </TouchableOpacity>

components/components/ImageCropView.js.js

        <PanGestureHandler onGestureEvent={handlePan}>
          <AnimatedImage
            style={imageStyle}
            source={{ uri: imageUri }}
          />
        </PanGestureHandler>

Does that answers your questions?

parasharrajat commented 2 years ago

The component supports images with a smaller size than canvas. It will just scale contain the canvas.

What will be scaled? Image or Canvas?

vitalii-tb commented 2 years ago

What will be scaled? Image or Canvas

The image will be scaled.

parasharrajat commented 2 years ago

Ok. Sounds good. Waiting for tgolen to finalize the discussion... I can't think of anything interesting as everything is UI-related.

What have you decided for Cropping the image on the Web? This would have to be done on Frontend.

tgolen commented 2 years ago

That answers most of my questions! Thank you for providing those details.

I'm curious about the web/desktop cropping too. That seems to be the only remaining detail.

parasharrajat commented 2 years ago

One point that I noticed is that we will have to use hooks to use the latest reanimated API. we don't use hooks normally. Am I thinking correctly @Tabakharniuk ? If yes, @tgolen Do you think we would be able to make a exception for these components.

tgolen commented 2 years ago

I believe our policy is that when integrating with third-party-libs, if there is no out-of-the-box solution except hooks, then it is an allowed exception.

parasharrajat commented 2 years ago

Ok. Thanks.

mallenexpensify commented 2 years ago

@parasharrajat @tgolen I haven't doubled for a month, any reason I shouldn't double the price meow?

parasharrajat commented 2 years ago

We already have good proposals. Just waiting for a few questions to be answered before development starts on this.

parasharrajat commented 2 years ago

Waiting for the questions...

vitalii-tb commented 2 years ago

@tgolen @parasharrajat, I am sorry for long reply.

On the web, it is a lot simpler than on iOS/Android, since we can use canvas. The function contract will be compatible with ImageManipulator.manipulateAsync API.

So generally, function for the web should have the following structure:

function cropImage (image, { originX, originY, width, height }){
  // create hidden canvas, which will not appear on the screen
  const canvas = document.createElement("canvas"); 
  const ctx = canvas.getContext("2d");

  // draw image on it
  const image = new Image()
  image.src = image;
  ctx.drawImage(image, width, originX, originY, 0, 0);

  // store new image
  const result = canvas.toDataURL("image/png");

  // remove canvas
  canvas.remove()
  return result;
}

Canvas and drawImage is supported on all your target platform, so there should not be any problem with the implementation

image

https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawImage

parasharrajat commented 2 years ago

Ok. Great. Thanks for the reply @Tabakharniuk. I am well aware of Ukraine's situation so I won't mind the delay.

I am fine with @Tabakharniuk 's proposal.

cc: @tgolen

:ribbon: :eyes: :ribbon: C+ reviewed

tgolen commented 2 years ago

Thank you for the additional detail. @mallenexpensify 🟢 to hire @Tabakharniuk for this and let's get to work!

mallenexpensify commented 2 years ago

The previous job was closed, I created a new one, can you apply here please @Tabakharniuk ? https://www.upwork.com/jobs/~01c481dcd538174d33

MelvinBot commented 2 years ago

📣 @Tabakharniuk You have been assigned to this job by @mallenexpensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

vitalii-tb commented 2 years ago

Thank you @tgolen @parasharrajat for the review.

The previous job was closed, I created a new one, can you apply here please @Tabakharniuk ?

Yes, sure, @mallenexpensify. I've just sent the proposal.

vitalii-tb commented 2 years ago

and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review

Realistically I need two weeks to finish work on the PR. ETA: 7 April 2022. Will that be acceptable? Or we need this feature faster?

parasharrajat commented 2 years ago

That works but we expect the PR to be pushed in a week with regular updates at intervals. Although, it is fine that PR takes some time before merging.

vitalii-tb commented 2 years ago

Okay, I will push the PR in a few days.