Page MenuHomePhabricator

[native] consolidate staff context provider into one file
ClosedPublic

Authored by ginsu on Jan 25 2024, 1:56 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 2, 2:20 PM
Unknown Object (File)
Fri, Jun 21, 3:02 AM
Unknown Object (File)
Apr 8 2024, 3:07 PM
Unknown Object (File)
Mar 16 2024, 8:43 AM
Unknown Object (File)
Mar 16 2024, 8:43 AM
Unknown Object (File)
Mar 16 2024, 8:43 AM
Unknown Object (File)
Mar 16 2024, 8:43 AM
Unknown Object (File)
Mar 16 2024, 8:40 AM
Subscribers

Details

Summary

As I was building the build info page for web, I noticed that the hasStaffUserLoggedIn only lives in native. To get this field into web we should lift the staff context provider into lib so that it can be used on both platforms.

As a preliminary step to that though, I noted that the staff context provider did not really follow the "pattern" that other context providers used where the context lives on top of the provider, and there is a hook called use____Context that the clients can use to use the provider. I really like this pattern that some of the other providers used and wanted to take a second to "tidy up" the staff context provider.

For example:

https://github.com/CommE2E/comm/blob/master/lib/components/modal-provider.react.js
https://github.com/CommE2E/comm/blob/master/web/tooltips/tooltip-provider.js
https://github.com/CommE2E/comm/blob/master/web/search/message-search-state-provider.react.js
https://github.com/CommE2E/comm/blob/master/lib/tunnelbroker/tunnelbroker-context.js

Depends on D10824

Test Plan

flow + confirmed that there were no regressions with native

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Jan 25 2024, 2:15 PM
atul requested changes to this revision.Jan 29 2024, 9:59 PM

Don't really understand the purpose of useStaffContext, would it make more sense to introduce something like useStaffUserHasBeenLoggedIn to consume the context?

native/staff/staff-context.provider.react.js
43–47 ↗(On Diff #36145)

This hook doesn't seem to be doing much, would it make more sense to introduce something like useStaffUserHasBeenLoggedIn?

This revision now requires changes to proceed.Jan 29 2024, 9:59 PM

Spoke to @atul IRL about this, but we decided that what I have is okay and even though useStaffContext isn't really doing too much it provides nice autocompletion for devs with fields for this provider in the code editor since we get to utilize the StaffContextType

provides nice autocompletion for devs

Feel like that's a good enough reason for me. We could probably simplify useStaffContext to use arrow syntax though

This revision is now accepted and ready to land.Jan 31 2024, 11:07 AM