Page MenuHomePhabricator

[native] introduce useNavigateToUserProfileBottomSheet hook
ClosedPublic

Authored by ginsu on Sep 21 2023, 11:59 AM.
Tags
None
Referenced Files
F2778195: D9261.diff
Fri, Sep 20, 7:44 AM
Unknown Object (File)
Fri, Sep 13, 2:57 AM
Unknown Object (File)
Fri, Sep 13, 2:57 AM
Unknown Object (File)
Fri, Sep 13, 2:57 AM
Unknown Object (File)
Fri, Sep 13, 2:57 AM
Unknown Object (File)
Fri, Sep 13, 2:57 AM
Unknown Object (File)
Fri, Sep 13, 2:56 AM
Unknown Object (File)
Fri, Sep 13, 2:50 AM
Subscribers

Details

Summary

This diff introduces the useNavigateToUserProfileBottomSheet hook which is going to be used to navigate to the user profile bottom sheet screen. This logic will need to be repeated in every place we want the user profiles to be accessible, so I thought that instead of copy+pasting the logic we should preemptively factor out this logic into this helper hook.

Depends on D9383

Test Plan

Confirmed that I can navigate to the user profiles screen using this hook

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.

changing stack order/will need to rebase

ginsu edited the summary of this revision. (Show Details)

update

native/user-profile/user-profile-utils.js
4 ↗(On Diff #31730)

I made this a debounce function to prevent the case where a user rapidly presses on a component and triggers this callback in rapid succession as the user profile bottom sheet comes into view. When this happens it could cause a render error due to too many re-renders. Adding the 500ms debounce delay here gives the navigator enough time to navigate to the user profile bottom sheet w/o any issues

native/user-profile/user-profile-utils.js
4 ↗(On Diff #31730)

_debounce forces a delay. I think you want _throttle

32 ↗(On Diff #31730)

I guess leading: true might be more like _throttle? If so, can you clarify the differences?

native/user-profile/user-profile-utils.js
32 ↗(On Diff #31730)

Yea you are right _throttle is going to be better here. The difference between throttling and debouncing is that the timer for the delay will reset every time the debounced function is called. Throttling won't do that. On the user's end it won't really make a difference, but will update to _throttle since that is technically more right

https://css-tricks.com/the-difference-between-throttling-and-debouncing/#aa-debouncing-enforces-that-a-function-not-be-called-again-until-a-certain-amount-of-time-has-passed-without-it-being-called-as-in-execute-this-function-only-if-100-milliseconds-have-passed-witho

replace _debounce with _throttle

native/user-profile/user-profile-utils.js
4 ↗(On Diff #31754)

I made this a throttled function to prevent the case where a user rapidly presses on a component and triggers this callback in rapid succession as the user profile bottom sheet comes into view. When this happens it could cause a render error due to too many re-renders. Adding the 500ms throttle delay here gives the navigator enough time to navigate to the user profile bottom sheet w/o any issues

atul added inline comments.
native/user-profile/user-profile-utils.js
30 ↗(On Diff #31754)

500ms seems kind of high? Trusting that you spent time trying this out and confirming that it was fine

This revision is now accepted and ready to land.Oct 9 2023, 12:29 PM
native/user-profile/user-profile-utils.js
30 ↗(On Diff #31754)

Yes confirmed that the opening + closing animation of the bottom sheet > 500ms, so by the time the user does a round trip the throttle delay will be over