[native] Move complexity of `actionSheetConfig` to `useShowAvatarActionSheet`
ClosedPublic
Actions

Authored by atul on Apr 18 2023, 3:32 PM.

Details

Reviewers

ashoat
ginsu

Commits

rCOMM38a498dbe6ce: [native] Move complexity of `actionSheetConfig` to `useShowAvatarActionSheet`

Summary

We can simplify the actionSheetConfig constructed in Edit*Avatar by moving some of what's common to useShowAvatarActionSheet. This cuts down on reptition a decent amount.

All of this refactoring brought the size of Edit*Avatar from 234 lines to 40:

This should make the subsequent changes easier to manage.

Depends on D7506

Test Plan

Everything looks and works as it did previously + ESLint + Flow

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Apr 18 2023, 3:32 PM

Herald added a subscriber: tomek. · View Herald TranscriptApr 18 2023, 3:32 PM

atul edited the summary of this revision. (Show Details)Apr 18 2023, 3:33 PM

atul published this revision for review.Apr 18 2023, 3:39 PM

atul added a child revision: D7508: [native] Move `UserAvatar` "inside" `EditUserAvatar`.Apr 18 2023, 3:47 PM

Harbormaster completed remote builds in B18484: Diff 25315.Apr 18 2023, 3:50 PM

Please address inline comments before landing! If you're unable to switch back to $ReadOnlyArray, please re-request review and I'll take a look

native/avatars/avatar-hooks.js
127 ↗	(On Diff #25315)	I don't think this function should be mutating the input array, and it doesn't look like it actually does. Can you try switching it back to `$ReadOnlyArray` and seeing if it has any type errors? Note that reassigning `options` is not the same as mutating the input array. Mutating input parameters is in my opinion an anti-pattern, since the caller isn't likely to expect it
171 ↗	(On Diff #25315)	I'm not a huge fan of the implicit `return undefined`... can we make it explicit, so it's clear that `cancel` doesn't get an icon?

This revision is now accepted and ready to land.Apr 19 2023, 6:03 AM

atul added inline comments.Apr 19 2023, 8:45 AM

native/avatars/avatar-hooks.js
127 ↗	(On Diff #25315)	I don't think this function should be mutating the input array, and it doesn't look like it actually does. Ah yeah, I totally blanked here. The reference to `options` is passed by value so if we were to eg append an element it would mutate the input but by spreading we're creating a new object and reassigning the "internal" meaning of `options` without mutating the object passed in as argument. Did a quick sanity check to make sure I understand things correctly: Will update this diff.