-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Logged out improvments #5771
Logged out improvments #5771
Conversation
|
src/components/AccountList.tsx
Outdated
<Text emoji style={[t.atoms.text_contrast_medium]}> | ||
{account.handle} | ||
</Text> | ||
<Text style={[t.atoms.text_contrast_medium]}>{account.handle}</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to sanitize the display name here since it's the user's own account, but the sanitizeHandle
util is used to insert the warning icon for invalid handles, and if that happens we need the emoji
prop. Pry good to add that sanitization for consistency, idk why it's not here already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok good to know
<ScrollView | ||
style={[a.h_full, a.w_full]} | ||
contentContainerStyle={{borderWidth: 0}}> | ||
<View | ||
style={[ | ||
a.px_2xl, | ||
{ | ||
paddingTop: isWeb ? 64 : insets.top, | ||
paddingTop: isWeb ? 64 : insets.top + 16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: feel like we could use a lil more padding here on native, and for consistency, could do like a.pt_2xl.paddingTop
or tokens.space._2xl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Android looks fine, iOS felt a little close to me but clearly it's gonna vary a bit, so not a huge deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumped to xl
, I think 2xl
might be a bit much even on iOS
src/view/com/auth/LoggedOut.tsx
Outdated
a.util_screen_outer, | ||
t.atoms.bg, | ||
{paddingTop: insets.top, paddingBottom: insets.bottom}, | ||
]}> | ||
<ErrorBoundary> | ||
{onDismiss && screenState === ScreenState.S_LoginOrCreateAccount ? ( | ||
<Pressable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could just swap in the Button
here if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice nice
Adds missing safe area insets to the signup and login flows. Also:
Test plan
Make sure there's no content leaking into the safe area for all the logged out screens, i.e. auth, deactivation, onboarding, etc