Page MenuHomePhabricator

[native] introduce loading logic to RelationshipPrompt
ClosedPublic

Authored by ginsu on Jun 27 2024, 2:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 7:05 PM
Unknown Object (File)
Sun, Nov 10, 5:13 PM
Unknown Object (File)
Sun, Nov 10, 5:07 PM
Unknown Object (File)
Sun, Nov 10, 4:31 PM
Unknown Object (File)
Sun, Nov 10, 3:31 PM
Unknown Object (File)
Sun, Nov 10, 2:57 PM
Unknown Object (File)
Sun, Nov 10, 2:40 PM
Unknown Object (File)
Sun, Nov 10, 1:22 PM
Subscribers

Details

Summary

This diff introduces the loading logic to RelationshipPrompt component

Depends on D12596

Test Plan

Confirmed that the relationship prompt buttons in the relationship prompt show a loading spinner when the action is in progress

Not loading:

Screenshot 2024-07-03 at 12.33.34 AM.png (1×944 px, 670 KB)

Screenshot 2024-07-03 at 12.33.49 AM.png (1×944 px, 667 KB)

Loading:

Screenshot 2024-07-03 at 12.33.37 AM.png (1×944 px, 666 KB)

Screenshot 2024-07-03 at 12.33.54 AM.png (1×944 px, 658 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

update

native/chat/relationship-prompt.react.js
169 ↗(On Diff #41776)

Hardcoded the height here so that we guarantee that when the button state goes to loading then the height of the button never changes; however, recently got some feedback about hardcoding the height in CSS being a no-no. Looks like visibility: hidden is not supported in react native, but if we still don't want this, I can explore other ways of doing this

This height was determined by using the element inspector in my simulator

forgot to hit save

native/chat/relationship-prompt.react.js
169 ↗(On Diff #41777)

Hardcoded the height here so that we guarantee that when the button state goes to loading then the height of the button never changes; however, recently got some feedback about hardcoding the height in CSS being a no-no. Looks like visibility: hidden is not supported in react native, but if we still don't want this, I can explore other ways of doing this

This height was determined by using the element inspector in my simulator

ginsu requested review of this revision.Jun 27 2024, 2:38 PM
ashoat requested changes to this revision.Jul 1 2024, 1:22 PM

Passing back for the same reasons as D12596. Using RelationshipButton everywhere seems like a good idea, but we should be thoughtful about the complexity if we end up having to render different things in different surfaces

This revision now requires changes to proceed.Jul 1 2024, 1:22 PM
ginsu retitled this revision from [native] use RelationshipButton in RelationshipPrompt to [native] introduce loading logic to RelationshipPrompt.Jul 2 2024, 9:38 PM
ginsu edited the summary of this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)

Using RelationshipButton everywhere seems like a good idea, but we should be thoughtful about the complexity if we end up having to render different things in different surfaces

I went ahead and abandoned this idea since the "Accept friend request" + "Reject friend request" text was too scrunched up in the button and in general it felt like this idea was creating a lot more added complexity

ashoat added inline comments.
native/chat/relationship-prompt.react.js
22 ↗(On Diff #41924)

How would you feel about naming this LoadableContent and factoring it out into native/components?

You could even combine it with Button to avoid having to pass isLoadingBlockUser twice. Imagine a "HOC" of Button:

type Props = {
  ...React.ElementConfig<typeof Button>,
  +isLoading: boolean,
};
function LoadableButton(props: Props): React.Node {
  const { isLoading, children, ...rest } = props;
  return (
    <Button
      disabled={isLoading}
      {...rest}
    >
      <ButtonContentContainer isLoading={isLoading}>
        {children}
      </ButtonContentContainer>
    </Button>
  );
}
105 ↗(On Diff #41924)

I know this issue wasn't introduced in this diff, but any chance you could memoize this array? (Can be reused below)

127 ↗(On Diff #41924)

And this one? (Can be reused below)

This revision is now accepted and ready to land.Jul 3 2024, 6:36 AM
native/chat/relationship-prompt.react.js
38–48 ↗(On Diff #41924)

Nit: personally I don't love this use of ternary syntax. See point 3 here

native/chat/relationship-prompt.react.js
22 ↗(On Diff #41924)

D12651

Introduced this diff at the start of the stack but will rebase and update this diff + D12596 so that those diffs are using LoadableButton

ginsu edited the test plan for this revision. (Show Details)

address comments

native/chat/relationship-prompt.react.js
105 ↗(On Diff #41924)

Forgot to address this, doing that rn

This revision was landed with ongoing or failed builds.Jul 3 2024, 3:13 PM
This revision was automatically updated to reflect the committed changes.