Page MenuHomePhabricator

[native] introduce loading logic to relationship prompt button
Needs ReviewPublic

Authored by ginsu on Thu, Jun 27, 1:02 PM.
Tags
None
Referenced Files
F2173149: D12596.diff
Tue, Jul 2, 9:37 PM
F2172911: Screenshot 2024-07-02 at 11.55.20 PM.png
Tue, Jul 2, 8:55 PM
F2172912: Screenshot 2024-07-02 at 11.55.27 PM.png
Tue, Jul 2, 8:55 PM
F2172907: Screenshot 2024-07-02 at 11.50.21 PM.png
Tue, Jul 2, 8:55 PM
F2172908: Screenshot 2024-07-02 at 11.50.42 PM.png
Tue, Jul 2, 8:55 PM
F2164527: D12596.diff
Mon, Jul 1, 11:49 PM
Unknown Object (File)
Sat, Jun 29, 1:48 AM
Unknown Object (File)
Sat, Jun 29, 1:48 AM
Subscribers

Details

Reviewers
inka
ashoat
Summary

This diff introduces the loading logic to the relationship prompt button.

Depends on D12595

Test Plan

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

Loading state:

Screenshot 2024-07-02 at 11.50.42 PM.png (1×944 px, 624 KB)

Screenshot 2024-07-02 at 11.50.21 PM.png (1×1 px, 789 KB)

Non loading state:

Screenshot 2024-07-02 at 11.55.20 PM.png (1×1 px, 802 KB)

Screenshot 2024-07-02 at 11.55.27 PM.png (1×1 px, 793 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu requested review of this revision.Thu, Jun 27, 1:18 PM
ginsu retitled this revision from [native] intorduce loading logic to relationship prompt button to [native] introduce loading logic to relationship prompt button.Thu, Jun 27, 1:19 PM
ginsu edited the summary of this revision. (Show Details)
ginsu added reviewers: inka, ashoat.
native/components/relationship-button.react.js
10

Introduced this new type with intention that this can be extended in the future with new sizes (M, L, XL, etc)

69

If these copy changes are good, will make sure I dedup the copy + pasted strings with D12595 before landing

93

Made the icon a bit smaller. It felt a little too big for me + looked a better with a smaller icon imo, but happy to change it back if people disagree

Before:

Screenshot 2024-06-27 at 4.31.11 PM.png (1×1 px, 789 KB)

Screenshot 2024-06-27 at 4.32.40 PM.png (1×1 px, 818 KB)

After:

Screenshot 2024-06-27 at 4.34.46 PM.png (1×1 px, 807 KB)

Screenshot 2024-06-27 at 4.35.24 PM.png (1×1 px, 828 KB)

native/user-profile/user-profile-relationship-button.react.js
99

The new small size makes the font of the button text a bit smaller so that we can fit all the elements nicely in the button

Before:

Screenshot 2024-06-27 at 4.41.25 PM.png (1×944 px, 660 KB)

After:

Screenshot 2024-06-27 at 4.41.29 PM.png (1×944 px, 652 KB)

158

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

ashoat requested changes to this revision.Mon, Jul 1, 1:20 PM
ashoat added inline comments.
native/components/relationship-button.react.js
69

I don't like the new, longer strings.

  1. We already say eg. "incoming friend request", so it's unambiguous what's being accepted/rejected/withdrawn
  2. The text looks too scrunched up
native/user-profile/user-profile-relationship-button.react.js
158

Let's remove this. The underlying issue is that the button changes height when loading.

The button changes height because one thing is unrendered (text) and another thing is rendered instead (spinner).

We should make the following changes:

  1. Always render one of these (eg. text) normally, but make it invisible when we don't want to see it
    • We can do this by changing its color to match the background as a baseline
    • But there might be a better way to do this (opacity: 0)
  2. Render the spinner with position: absolute
    • Don't bother rendering it when not necessary
169

Same here

This revision now requires changes to proceed.Mon, Jul 1, 1:20 PM