-
Notifications
You must be signed in to change notification settings - Fork 109
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
PIN : unlock screen ui #1624
PIN : unlock screen ui #1624
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1624 +/- ##
===========================================
+ Coverage 58.98% 59.14% +0.15%
===========================================
Files 1225 1228 +3
Lines 31619 31868 +249
Branches 6493 6557 +64
===========================================
+ Hits 18652 18849 +197
- Misses 10143 10164 +21
- Partials 2824 2855 +31
☔ View full report in Codecov by Sentry. |
…ontain `ElementTheme` composable, and fix existing issues.
…re internal, and fix existing issues.
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 work!
Some remarks, that may be handed later.
Also I pushed 2 commits and triggered a record screenshot action.
@@ -50,14 +50,36 @@ data class PinEntry( | |||
return copy(digits = newDigits.toPersistentList()) | |||
} | |||
|
|||
fun deleteLast(): PinEntry { |
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.
Not from this PR, but for clarity, maybe rename the fun fun empty(size: Int)
in the companion to create
, or createEmpty
?
} | ||
return copy(digits = newDigits.toPersistentList()) | ||
} | ||
|
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 the method below clear()
is not correct (but it's not used).
It should return PinEntry.empty(size)
Maybe adding unit test to cover PinEntry
behavior could help.
@Composable | ||
override fun present(): PinUnlockState { | ||
var pinEntry by remember { | ||
mutableStateOf(PinEntry.empty(4)) |
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.
Use PIN_SIZE
instead of hard-coded 4?
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 that is the number of digit is "dynamic" (for instance config server side), we should store the size of the pin code when it has been setup, or the user will be stuck if the config change.
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.
The size will be loaded dynamically when I'll branch the logic
mutableStateOf(PinEntry.empty(4)) | ||
} | ||
var remainingAttempts by rememberSaveable { | ||
mutableIntStateOf(3) |
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 remainingAttempts
has to be saved on disk. You will do it later?
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.
We have all the methods, I just need to branch the logic, but it's part of another PR
is PinUnlockEvents.OnPinKeypadPressed -> { | ||
pinEntry = pinEntry.process(event.pinKeypadModel) | ||
if (pinEntry.isComplete()) { | ||
coroutineScope.launch { pinStateService.unlock() } |
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 to do later, (just adding a reminder): compare with the correct PIN code.
Maybe safer to add TODOs in the code...
private val maxSizePinKey = 80.dp | ||
|
||
@Composable | ||
fun PinKeypad( |
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.
numpad (as the package name) or keypad? Maybe keep only one single name.
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 will change the package
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Pin unlock UI for https://www.figma.com/file/0MMNu7cTOzLOlWb7ctTkv3/Element-X?type=design&node-id=13067-107540&mode=dev
There is no logic yet, it's coming soon.