Skip to content

Commit

Permalink
[lib] Make displayFailureAlert optional in BaseEditUserAvatar
Browse files Browse the repository at this point in the history
Summary:
`displayFailureAlert` is native-specific functionality which we won't implement for `web`.

We previously had an implementation using `alert(...)`, but showing a browser alert was not consistent with the rest of `web`'s error handling. It makes sense to have `displayFailureAlert` be optional and only implement it for `native`. Later in the stack we will handle this from `native/.../avatar-hooks.js` instead.

NOTE: The higher level goal with this (and subsequent) refactor(s) is for `[web/native]/.../avatar-hooks.react.js` to be platform-specific and entity(user vs. thread)-agnostic AND for `edit-*-avatar-provider.react.js` to be platform-agnostic and entity-specific. Right now there's some coupling between the two that makes implementation of image avatars on `web` pretty difficult... will work on cleaning things up in this stack. TLDR: avatar providers will be common interface between client and keyserver and avatar hooks will be platform-specific implementations to get things from platform-specific types (eg `MediaLibrarySelection`) to platform-agnostic types (eg `UpdateUserAvatarRequest`) so they can be passed to API provided by providers.

---

Depends on D8315

Test Plan: Alerts continue to appear as expected on `native`. Alerts continue NOT to appear on `web` as expected.

Reviewers: ashoat, ginsu, rohan

Reviewed By: ginsu

Subscribers: tomek

Differential Revision: https://phab.comm.dev/D8316
  • Loading branch information
atulsmadhugiri committed Jun 27, 2023
1 parent 9bfaa6d commit 2dabda2
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 6 deletions.
6 changes: 3 additions & 3 deletions lib/components/base-edit-user-avatar-provider.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const updateUserAvatarLoadingStatusSelector = createLoadingStatusSelector(
);

type Props = {
+displayFailureAlert: () => mixed,
+displayFailureAlert?: () => mixed,
+selectFromGallery: () => Promise<?MediaLibrarySelection>,
+useUploadSelectedMedia: (
setProcessingOrUploadInProgress?: (inProgress: boolean) => mixed,
Expand Down Expand Up @@ -106,7 +106,7 @@ function BaseEditUserAvatarProvider(props: Props): React.Node {
try {
return await updateUserAvatarCall(imageAvatarUpdateRequest);
} catch (e) {
displayFailureAlert();
displayFailureAlert && displayFailureAlert();
throw e;
}
})();
Expand Down Expand Up @@ -144,7 +144,7 @@ function BaseEditUserAvatarProvider(props: Props): React.Node {
try {
return await updateUserAvatarCall(request);
} catch (e) {
displayFailureAlert();
displayFailureAlert && displayFailureAlert();
throw e;
}
})();
Expand Down
3 changes: 0 additions & 3 deletions web/avatars/web-edit-user-avatar-provider.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import * as React from 'react';

import { BaseEditUserAvatarProvider } from 'lib/components/base-edit-user-avatar-provider.react.js';

const displayAvatarUpdateFailureAlert = () => null;

// TODO: Implement `selectFromGallery(...)` for `web`.
const selectFromGallery = async () => null;

Expand All @@ -19,7 +17,6 @@ function WebEditUserAvatarProvider(props: Props): React.Node {
const { children } = props;
return (
<BaseEditUserAvatarProvider
displayFailureAlert={displayAvatarUpdateFailureAlert}
selectFromGallery={selectFromGallery}
useUploadSelectedMedia={useUploadSelectedMedia}
>
Expand Down

0 comments on commit 2dabda2

Please sign in to comment.