Page MenuHomePhabricator

[lib] introduce loading logic to useRelationshipPrompt
AcceptedPublic

Authored by ginsu on Thu, Jun 27, 12:18 PM.
Tags
None
Referenced Files
F2174184: D12593.diff
Tue, Jul 2, 11:43 PM
F2167678: D12593.diff
Tue, Jul 2, 6:55 AM
Unknown Object (File)
Mon, Jul 1, 8:42 PM
Unknown Object (File)
Mon, Jul 1, 9:12 AM
Unknown Object (File)
Sun, Jun 30, 3:47 PM
Unknown Object (File)
Sat, Jun 29, 2:34 AM
Unknown Object (File)
Sat, Jun 29, 2:34 AM
Unknown Object (File)
Sat, Jun 29, 2:34 AM
Subscribers

Details

Reviewers
inka
ashoat
Summary

This diff introduces the loading logic needed for the relationship prompts on both web and native. Initially, I thought about using createLoadingStatusSelector to implement the loading logic; however, the issue with this was that we sometimes show two relationship buttons and using a single createLoadingStatusSelector would cause both button to show loading spinners at the same time even though one button was pressed, which I felt was a bit strange. So I introduced 4 separate loading states (one for each action) and returned those values in this hook to be consumed on both web + native

Test Plan

At the end of the stack confirmed that the relationship prompt buttons were being disabled + showing some sort of visual indication to the user that the action is loading

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu added a reviewer: inka.
ashoat requested changes to this revision.Mon, Jul 1, 12:44 PM

We shouldn't set all loading states to false when just one resolves

lib/hooks/relationship-prompt.js
38

Can we rename loaders to inProgress or loadingState? When I read "loader" I think of a function that will get called to do some loading

79

Can we rename loaders to inProgress or loadingState? When I read "loader" I think of a function that will get called to do some loading

102–105

It's a bit weird that we set all of these to false here. Can we try this instead?

const updateRelationship = React.useCallback(
  async (action: TraditionalRelationshipAction, setInProgress: boolean => mixed) => {
    try {
      setInProgress(true);
      invariant(otherUserID, 'Other user info id should be present');
      return await callUpdateRelationships({
        action,
        userIDs: [otherUserID],
      });
    } catch (e) {
      onErrorCallback?.();
      throw e;
    } finally {
      setInProgress(false);
    }
  },
  [callUpdateRelationships, onErrorCallback, otherUserID],
);

const dispatchActionPromise = useDispatchActionPromise();
const onButtonPress = React.useCallback(
  (action: TraditionalRelationshipAction, setInProgress: boolean => mixed) => {
    void dispatchActionPromise(
      updateRelationshipsActionTypes,
      updateRelationship(action, setInProgress),
    );
  },
  [dispatchActionPromise, updateRelationship],
);

const blockUser = React.useCallback(
  () => onButtonPress(relationshipActions.BLOCK, setIsLoadingBlockUser),
  [onButtonPress],
);
const unblockUser = React.useCallback(
  () => onButtonPress(relationshipActions.UNBLOCK, setIsLoadingUnblockUser),
  [onButtonPress],
);
const friendUser = React.useCallback(
  () => onButtonPress(relationshipActions.FRIEND, setIsLoadingFriendUser),
  [onButtonPress],
);
const unfriendUser = React.useCallback(
  () => onButtonPress(relationshipActions.UNFRIEND, setIsLoadingUnfriendUser),
  [onButtonPress],
);
This revision now requires changes to proceed.Mon, Jul 1, 12:44 PM
ashoat added inline comments.
lib/hooks/relationship-prompt.js
126 ↗(On Diff #41862)

Can you move the setInProgress(true); call to the start of updateRelationship, like in my code snippet from the last review?

This revision is now accepted and ready to land.Mon, Jul 1, 5:52 PM