Paths

Table of Contentst

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

Authored by atul on Apr 17 2023, 3:21 PM.

Details

Reviewers

ashoat
ginsu

Commits

rCOMM8ce8efd3731d: [native] Introduce `openPhotoGallery` in `EditAvatar` component

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

Processing the selected media
Uploading the selected media
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.

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D7476 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 17 2023, 3:21 PM

Herald added a subscriber: tomek. · View Herald TranscriptApr 17 2023, 3:21 PM

atul added inline comments.Apr 17 2023, 3:25 PM

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 edited the test plan for this revision. (Show Details)Apr 17 2023, 3:27 PM

Harbormaster completed remote builds in B18421: Diff 25234.Apr 17 2023, 3:39 PM

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:

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:

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

Changes needed to be made to to processMedia in order for us to call it from EditAvatar.
Ginsu is actively refactoring EditAvatar so I wanted to keep my changes as isolated as possible from the action sheet/etc.
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 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. 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?

address feedback

rebase and land

atul added a child revision: D7481: [native] Process and upload image for image avatar in `EditAvatar`.Apr 18 2023, 8:01 AM

Harbormaster completed remote builds in B18447: Diff 25264.Apr 18 2023, 8:12 AM

This revision was landed with ongoing or failed builds.Apr 18 2023, 8:12 AM

Closed by commit rCOMM8ce8efd3731d: [native] Introduce `openPhotoGallery` in `EditAvatar` component (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM8ce8efd3731d: [native] Introduce `openPhotoGallery` in `EditAvatar` component.

Harbormaster completed remote builds in B18450: Diff 25267.Apr 18 2023, 8:18 AM

ashoat added inline comments.Apr 18 2023, 9:17 AM

native/components/edit-avatar.react.js
64 ↗	(On Diff #25270)	`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