Page MenuHomePhabricator

[lib] introduce loading logic to useRelationshipPrompt
ClosedPublic

Authored by ginsu on Thu, Jun 27, 12:18 PM.
Tags
None
Referenced Files
F2194660: D12593.id41765.diff
Fri, Jul 5, 6:38 AM
F2189087: D12593.id41767.diff
Thu, Jul 4, 10:08 AM
Unknown Object (File)
Wed, Jul 3, 2:17 PM
Unknown Object (File)
Wed, Jul 3, 2:17 PM
Unknown Object (File)
Wed, Jul 3, 1:30 PM
Unknown Object (File)
Wed, Jul 3, 10:46 AM
Unknown Object (File)
Tue, Jul 2, 11:43 PM
Unknown Object (File)
Tue, Jul 2, 6:55 AM
Subscribers

Details

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

Depends on D12651

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 ↗(On Diff #41765)

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 ↗(On Diff #41765)

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 ↗(On Diff #41765)

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
This revision was landed with ongoing or failed builds.Wed, Jul 3, 3:13 PM
This revision was automatically updated to reflect the committed changes.