Page MenuHomePhabricator
Differential D5514

[native] Add `staffUserHasBeenLoggedInIndicator` to `LoggedInModal`
ClosedPublic
Actions

Authored by atul on Oct 31 2022, 2:52 PM.
Tags
None
Referenced Files
F3382750: D5514.id18006.diff
Thu, Nov 28, 11:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Mon, Nov 25, 3:50 AM
Unknown Object (File)
Tue, Nov 19, 4:13 PM
Unknown Object (File)
Tue, Nov 19, 4:13 PM
View All Files
Subscribers
• abosh

Details

Reviewers
ashoat
tomek
varun
ginsu
rohan
Commits
rCOMM1e7e1a673f7d: [native] Add `staffUserHasBeenLoggedInIndicator` to `LoggedInModal`
Summary

Context: https://linear.app/comm/issue/ENG-2134/have-a-flag-that-indicates-whether-the-build-is-a-staff-release

Consumes StaffContext and displays whether a staff user has logged in only if staffCanSee is true.

(Will restyle a bit after diff that adds staffUserHasBeenLoggedIn indicator to Build Info page of Profile tab.)

Test Plan

Here's what it looks like after a staff user has logged in and subsequently logged out:

f0fe25.png (1×696 px, 616 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D5514 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Oct 31 2022, 2:52 PM
Herald added a subscriber: • abosh. · View Herald TranscriptOct 31 2022, 2:52 PM
atul edited the test plan for this revision. (Show Details)Oct 31 2022, 2:53 PM
atul updated this revision to Diff 18007.Oct 31 2022, 2:54 PM

remove onPress={undefined} for staffUserHasBeenLoggedInIndicator

Harbormaster completed remote builds in B13117: Diff 18006.Oct 31 2022, 3:07 PM
Harbormaster completed remote builds in B13118: Diff 18007.
atul requested review of this revision.Oct 31 2022, 3:07 PM
ashoat accepted this revision.Nov 1 2022, 5:28 AM

Accepting since my comments can be addressed in a later diff

native/account/logged-out-modal.react.js
472 ↗(On Diff #18007)

I'd like to turn this into an ||, but arguably it would make more sense if the context had a single parameter that handled this union. See my comments in D5513 for additional context

475 ↗(On Diff #18007)

See my comments in D5513:

  1. Can we say "ADVANCED LOGGED ENABLED" instead?
  2. Can we make this button actually show an Alert when pressed?
This revision is now accepted and ready to land.Nov 1 2022, 5:28 AM
atul updated this revision to Diff 18058.Nov 3 2022, 10:59 AM

rebase before landing

Harbormaster completed remote builds in B13157: Diff 18058.Nov 3 2022, 11:13 AM
atul added a comment.Nov 3 2022, 1:52 PM

Accepting since my comments can be addressed in a later diff

Will land as-is and address feedback in followups: https://linear.app/comm/issue/ENG-2134#comment-52a4e6fa

Closed by commit rCOMM1e7e1a673f7d: [native] Add `staffUserHasBeenLoggedInIndicator` to `LoggedInModal` (authored by atul). · Explain WhyNov 3 2022, 2:02 PM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM1e7e1a673f7d: [native] Add `staffUserHasBeenLoggedInIndicator` to `LoggedInModal`.

Revision Contents
Changeset List

PathSize
native/
account/
19 lines

Diff 18058

View Options

native/account/logged-out-modal.react.js

Loading...