-
Notifications
You must be signed in to change notification settings - Fork 150
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
Always display 'lost recovery key?' option #2745
Always display 'lost recovery key?' option #2745
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2745 +/- ##
===========================================
+ Coverage 73.55% 73.56% +0.01%
===========================================
Files 1501 1501
Lines 35987 35992 +5
Branches 6940 6940
===========================================
+ Hits 26471 26479 +8
+ Misses 5869 5867 -2
+ Partials 3647 3646 -1 ☔ View full report in Codecov by Sentry. |
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.
Thanks!
A few comments.
) | ||
} | ||
BottomMenu( | ||
positiveButtonTitle = stringResource(R.string.screen_identity_use_another_device), |
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 do not think we should propose to "Use another device"
if we know that this is the latest session (isLastDevice
). Maybe the UX can be better here by just hiding the first button?
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 call!
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.
Done in ccf3c2a.
@@ -29,7 +29,7 @@ data class VerifySelfSessionState( | |||
) { | |||
@Stable | |||
sealed interface VerificationStep { | |||
data class Initial(val canEnterRecoveryKey: Boolean, val isLastDevice: Boolean) : VerificationStep | |||
data class Initial(val canEnterRecoveryKey: Boolean) : VerificationStep |
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.
Maybe revert this change, see my other comment.
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.
Also done in ccf3c2a.
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 think we should not use the term passcode
, this is element-hq/element-meta#2402 and since this is affecting the same preview, maybe handle it in the same PR?
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 think this is only the 4th time we've changed this screen in the past couple of weeks. I guess 5th time's the charm!
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.
5b84eb7 + a new recording of screenshots should fix it.
Use it to display only 'enter recovery key' option for verification.
This should fix the wrong term 'passcode' being used in the recovery key screen title.
…play-lost-recovery-key-action
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.
Thanks for the changes. One final remark, then I think we're good!
text = stringResource(id = R.string.screen_recovery_key_confirm_lost_recovery_key), | ||
modifier = Modifier.fillMaxWidth(), | ||
onClick = onCreateRecoveryKey, | ||
) |
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 think this button should be disabled when state.submitAction.isLoading()
. WDYT?
Made this remark when seeing this screenshot: https://github.com/element-hq/element-x-android/pull/2745/files#diff-dbd46661ff7c38b3aefb7d016c5a6e54348191cc5ac6a6f1bd71213ad7db2019
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.
Actually, when the progress indicator is displayed the button is not clickable, so it's 'disabled' although it doesn't display the disabled state. It's what we have everywhere else I believe.
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, for "Continue" button, but I am talking about the other one "Lost your recovery key?". Sorry if it was not clear.
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 true, good call!
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.
So to clarify:
The two buttons must be disabled during the submit action loading, to avoid user navigating away during the process.
Adding at line 101:
enabled = !state.submitAction.isLoading()
should be fine.
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.
Thanks!
Quality Gate passedIssues Measures |
Type of change
Content
Motivation and context
Should be enough to close #2740. It also applies the latest changes to the designs in Figma, which changed the behaviour mentioned above.
Screenshots / GIFs
In the PR.
Tests
Tested devices
Checklist