-
Notifications
You must be signed in to change notification settings - Fork 369
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
[Wallet] Various onboarding fixes #4213
Conversation
- Fix insufficient fund error briefly shown while fetching the balance after enter the invide code - Hide back button when no back action is possible - Tweak VerificationLoadingScreen and Interstitial with new colors and fonts
@@ -43,7 +44,7 @@ class VerificationInterstitialScreen extends React.Component<WithTranslation> { | |||
<AnimatedCircle /> | |||
<AnimatedCircle /> | |||
</View> | |||
<Text style={fontStyles.h1}>{t('interstitial.header')}</Text> | |||
<Text style={fontStyles.h2}>{t('interstitial.header')}</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.
nit: Define in stylesheet?
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.
@tarikbellamine There's not reason to redefine this in the stylesheet if we don't deviate from the predefined value in fontStyles.
@@ -19,7 +20,7 @@ const AnimatedCircle = () => ( | |||
) | |||
|
|||
class VerificationInterstitialScreen extends React.Component<WithTranslation> { | |||
static navigationOptions = { gestureEnabled: false, header: null } | |||
static navigationOptions = noHeaderGestureDisabled |
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.
Does defining navigation options like this still work as expected?
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.
Yes, tested successfully.
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.
Good stuff! Left small questions but think this is pretty much good to go once the tests are passing
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.
LGTM! A few small comments below :)
const { t } = useTranslation(Namespaces.onboarding) | ||
const userBalance = useSelector((state) => state.stableToken.balance) | ||
const balanceIsSufficient = isUserBalanceSufficient(userBalance, VERIFICATION_FEE_ESTIMATE) | ||
// For now only show error if user has pressed start | ||
// with the idea being that by the time the user is done reading the screen the balance will already be known | ||
const showError = hasPressedStart && !balanceIsSufficient | ||
const dispatch = useDispatch() | ||
const headerHeight = useHeaderHeight() | ||
const insets = useSafeArea() | ||
|
||
function onPressStart() { |
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: These should be arrow functions
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.
Yes 👍
@@ -43,7 +44,7 @@ class VerificationInterstitialScreen extends React.Component<WithTranslation> { | |||
<AnimatedCircle /> | |||
<AnimatedCircle /> | |||
</View> | |||
<Text style={fontStyles.h1}>{t('interstitial.header')}</Text> | |||
<Text style={fontStyles.h2}>{t('interstitial.header')}</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.
@tarikbellamine There's not reason to redefine this in the stylesheet if we don't deviate from the predefined value in fontStyles.
@@ -123,8 +126,8 @@ class VerificationLoadingScreen extends React.Component<Props> { | |||
<View style={styles.buttonCancelContainer}> | |||
<CancelButton onCancel={this.onCancel} /> | |||
</View> | |||
<DevSkipButton nextScreen={Screens.VerificationInterstitialScreen} /> |
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.
I can't remember if this is my bug or someone else's but this screen shouldn't have a DevSkipButton. It won't work as expected with verification ongoing in the background. If devs want to skip they should just choose skip on the education screen
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.
Makes sense yes, removing then 👍
Description
Tested
Yes ;-)
Related issues
Backwards compatibility
Yes