Skip to content
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

Feature/fga/pin create UI #1608

Merged
merged 17 commits into from
Oct 20, 2023
Merged

Feature/fga/pin create UI #1608

merged 17 commits into from
Oct 20, 2023

Conversation

ganfra
Copy link
Contributor

@ganfra ganfra commented Oct 19, 2023

First screen of the PIN creation flow.
There is no logic branched yet to save the pin.

Also I've not yet tested the interactions.

Figma : https://www.figma.com/file/0MMNu7cTOzLOlWb7ctTkv3/Element-X?type=design&node-id=13067-150738

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/LgqJhw

@ganfra ganfra marked this pull request as ready for review October 19, 2023 15:58
@ganfra ganfra requested a review from a team as a code owner October 19, 2023 15:58
@ganfra ganfra requested review from bmarty and removed request for a team October 19, 2023 15:58
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (46f78ef) 58.82% compared to head (4327689) 58.96%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1608      +/-   ##
===========================================
+ Coverage    58.82%   58.96%   +0.14%     
===========================================
  Files         1222     1226       +4     
  Lines        31438    31630     +192     
  Branches      6443     6494      +51     
===========================================
+ Hits         18493    18652     +159     
- Misses       10145    10154       +9     
- Partials      2800     2824      +24     
Files Coverage Δ
...features/lockscreen/impl/create/CreatePinEvents.kt 100.00% <100.00%> (+100.00%) ⬆️
.../features/lockscreen/impl/create/CreatePinState.kt 100.00% <100.00%> (ø)
...s/lockscreen/impl/create/CreatePinStateProvider.kt 96.29% <100.00%> (+16.29%) ⬆️
.../features/lockscreen/impl/create/model/PinDigit.kt 100.00% <100.00%> (ø)
...kscreen/impl/create/validation/CreatePinFailure.kt 100.00% <100.00%> (ø)
.../lockscreen/impl/create/validation/PinValidator.kt 100.00% <100.00%> (ø)
...droid/libraries/designsystem/theme/ColorAliases.kt 80.76% <100.00%> (+2.50%) ⬆️
.../features/lockscreen/impl/create/model/PinEntry.kt 87.50% <87.50%> (ø)
...tures/lockscreen/impl/create/CreatePinPresenter.kt 83.33% <86.84%> (+83.33%) ⬆️
...d/features/lockscreen/impl/create/CreatePinView.kt 62.50% <67.02%> (+15.83%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some remarks on the wording

<string name="screen_app_lock_setup_confirm_pin">"Confirm PIN"</string>
<string name="screen_app_lock_setup_pin_blacklisted_dialog_content">"You cannot choose this as your PIN code for security reasons"</string>
<string name="screen_app_lock_setup_pin_blacklisted_dialog_title">"Choose a different PIN"</string>
<string name="screen_app_lock_setup_pin_context">"Lock Element to add extra security to your chats."</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to make the app name configurable I think

Suggested change
<string name="screen_app_lock_setup_pin_context">"Lock Element to add extra security to your chats."</string>
<string name="screen_app_lock_setup_pin_context">"Lock %s$1 to add extra security to your chats."</string>

<string name="screen_app_lock_settings_remove_pin">"Remove PIN"</string>
<string name="screen_app_lock_settings_remove_pin_alert_message">"Are you sure you want to remove PIN?"</string>
<string name="screen_app_lock_settings_remove_pin_alert_title">"Remove PIN?"</string>
<string name="screen_app_lock_setup_choose_pin">"Choose %1$d digit PIN"</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(maybe change to "Choose PIN" to fix this)

<?xml version="1.0" encoding="utf-8"?>
<resources xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2">
<plurals name="screen_app_lock_subtitle_wrong_pin">
<item quantity="one">"Wrong PIN. You have %1$d more chance"</item>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amshakal do we want to put something more alarming here? We had this in EA currently, for the last attempt:

<string name="wrong_pin_message_last_remaining_attempt">Warning! Last remaining attempt before logout!</string>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Ill craft something a bit more alarming, thank you!

@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ganfra ganfra merged commit ef08d77 into develop Oct 20, 2023
15 checks passed
@ganfra ganfra deleted the feature/fga/pin_create_ui branch October 20, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants