Page MenuHomePhabricator

[native] Introduce `openPhotoGallery` in `EditAvatar` component
ClosedPublic

Authored by atul on Apr 17 2023, 3:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 4:38 AM
Unknown Object (File)
Wed, Nov 6, 3:29 AM
Unknown Object (File)
Wed, Nov 6, 3:29 AM
Unknown Object (File)
Wed, Nov 6, 3:29 AM
Unknown Object (File)
Tue, Nov 5, 4:30 AM
Unknown Object (File)
Tue, Nov 5, 4:30 AM
Unknown Object (File)
Tue, Nov 5, 2:10 AM
Unknown Object (File)
Thu, Oct 17, 2:33 PM
Subscribers

Details

Summary

This was largely copy/pasted from @ginsu's work here: https://gist.github.com/ginsueddy/8f0dc9f2033e1a3c61282a060527814f

For now this callback just opens the native OS image picker and constructs MediaLibrarySelection.

The MediaLibrarySelection will then be passed to processMedia (which conveniently takes NativeMediaSelection (which is an alias for MediaLibrarySelection | etc) and MediaProcessConfig.

Subsequent diffs will handle

  1. Processing the selected media
  2. Uploading the selected media
  3. Constructing avatar update request with server uploadID
Test Plan

Replace onPress for "Use Emoji" button with openPhotoGallery to ensure that media gallery launches and selection is constructed correctly.

d71305.png (2×3 px, 3 MB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/components/edit-avatar.react.js
66 ↗(On Diff #25234)

This is just to appease ESLint, will instead imperatively call something like processSelectedMedia (or maybe set some sort of state via setLocalMediaSelection and have processing code triggered declaratively?)

atul requested review of this revision.Apr 17 2023, 3:39 PM

I really, really do not like that this function is getting introduced without a callsite.

  • It's hard for me to answer, eg. will the callsite have a problem with you returning ?MediaLibrarySelection
  • You're forcing yourself to introduce and then remove eslint-disable-next-line
  • I had to scroll through the rest of your stack to try and find where this function is used, and couldn't find any place
  • I had to read through your test plan and the rest of the file to try and find where the callsite would be
  • Nowhere in the diff did you even attempt to explain why you're introducing a function (a small one, at that) without any usage or unit tests anywhere

In literally any other scenario I would request changes, but given the state of this work that's basically not an option. Please take my feedback seriously... I strongly feel that this is an anti-pattern.

native/components/edit-avatar.react.js
61–62 ↗(On Diff #25234)

Why aren't these set to Date.now()? I'm guessing you copied the code in MediaGalleryKeyboard, but it's set to 0 only temporarily, and gets set to the current time in sendMedia.

In this case, it's impossible to tell without seeing where this function is used, but I don't think it makes sense to temporarily set to 0 and then set to Date.now() later... I think we should set to Date.now() immediately.

88 ↗(On Diff #25234)

Why didn't you simply add something to options that triggers your new function onPress?

Cons:

  • It would take a couple minutes to do this, but it's not like you won't need to do it in the future anyways

Pros:

  • You would make it easier for me to review your diff
    • I would not feel like I have to tab through all your diffs in search of where this function is used
    • I would not need to spend time trying to piece together where the new function will eventually be used (I concluded it's here)
    • I would not feel like I need to spend time writing this essay, which mostly just reiterates points I've already made to you
  • You would not need to introduce (and then eventually remove) eslint-disable-next-line
  • You would make it easier for others to test your diff
    • It would only be necessary to patch this diff to test it, instead of needing to follow the loose patch instructions in your Test Plan
  • You would reduce the amount of conflict on the team

It's hard to imagine why you're still insisting in introducing these naked functions, and not even sharing an explanation for it...

This revision is now accepted and ready to land.Apr 18 2023, 5:16 AM

will the callsite have a problem with you returning ?MediaLibrarySelection

I mention in the description exactly how we'll use the ?MediaLibrarySelection:

d571bf.png (156×1 px, 43 KB)

You're forcing yourself to introduce and then remove eslint-disable-next-line

That's fair, I also tend to dislike temporary flow/eslint/etc ignore directives... but didn't think it was a huge deal here.

I had to scroll through the rest of your stack to try and find where this function is used, and couldn't find any place

Right now I'm testing the image upload functionality by swapping out the "Use Emoji" onPress with openPhotoGallery as described in the Test Plan. I thought it would be clear that once complete there would be a new entry in options for Select image.

I had to read through your test plan and the rest of the file to try and find where the callsite would be

In the future I'll be more explicit and put it in the Summary instead of the Test Plan

Nowhere in the diff did you even attempt to explain why you're introducing a function (a small one, at that) without any usage or unit tests anywhere

Even though it's not used, I explained how it will be used:

1608dc.png (364×2 px, 91 KB)

As to why it's not used in this diff:

  1. Changes needed to be made to to processMedia in order for us to call it from EditAvatar.
  2. Ginsu is actively refactoring EditAvatar so I wanted to keep my changes as isolated as possible from the action sheet/etc.
  3. The feature isn't ready. While the EditAvatar component is behind a staffCanSee check, this would still be a broken experience for staff in eg a staff release. I was just going to keep pointing "Use emoji" to openPhotoGallery until the functionality was complete, and then add an entry to editAvatarOptions to "hook it up."

I'll be more explicit about reasoning for diff size/function usage in Summary section going forward.

native/components/edit-avatar.react.js
61–62 ↗(On Diff #25234)

Makes sense, will set to Date.now().

88 ↗(On Diff #25234)

Why didn't you simply add something to options that triggers your new function onPress?

Two main reasons

  1. Ginsu is actively working on refactoring EditAvatar so I wanted my changes in this (actively changing) file to be as isolated as possible to make "joining" our work as seamless as possible when it comes time. I discussed this with Ginsu, but I should have called it out explicitly in the Summary of the diff.
  2. The functionality is not complete. As of this diff I tested and made sure the image selection worked as expected, but we don't yet do anything with the selection. Even though the EditAvatar component is behind a staffCanSee check, it would still be a broken experience for those on eg staff releases.

You would make it easier for others to test your diff

  • It would only be necessary to patch this diff to test it, instead of needing to follow the loose patch instructions in your Test Plan

I wouldn't say they're loose patch instructions, they're pretty specific about what change needs to be made (replacing onPressEmojiAvatarFlow with openPhotoGallery).

If a reviewer is already going through the trouble to patch a diff, swapping out a function name should be a negligible amount of effort. There are plenty of diffs that have been orders of magnitude more difficult to test (eg requiring reviewer to patch in a bunch of changes from Gists, changing a bunch of booleans in various files, anything that requires an iOS/Android build etc).

It's hard to imagine why you're still insisting in introducing these naked functions, and not even sharing an explanation for it...

Is this just additional commentary, or are you expecting some sort of response here?

This revision was landed with ongoing or failed builds.Apr 18 2023, 8:12 AM
This revision was automatically updated to reflect the committed changes.
native/components/edit-avatar.react.js
64

retries should not be set to currentTime. This is an integer count of the number of retries that have occurred already, and should always be initialized to 0

native/components/edit-avatar.react.js
64