[PM-17531] Add dialog for client certificate import #4622
Conversation
SaintPatrck
requested review from
brian-livefront,
david-livefront,
dseverns-livefront,
ahaisting-livefront,
phil-livefront and
a team
as code owners
|
Great job, no security vulnerabilities found in this Pull Request |
| value = password, | ||
| onValueChange = { password = it }, | ||
| modifier = Modifier | ||
| .testTag("AlertClientCertificatePasswordInputField") |
There was a problem hiding this comment.
For both the this and the BitwardenTextField above, we need to use the textFieldTestTag and passwordFieldTestTag instead of the modifier directly. You may need to rebase, for the password one as it is new
| label = stringResource(R.string.alias), | ||
| value = alias, | ||
| onValueChange = { alias = it }, | ||
| modifier = Modifier |
There was a problem hiding this comment.
Can we put the modifier at the bottom?
| @Composable | ||
| fun BitwardenClientCertificateDialog( | ||
| onConfirmClick: (alias: String, password: String) -> Unit, | ||
| onDismissRequest: () -> Unit, |
There was a problem hiding this comment.
Can we add a modifier parameter with default here
There was a problem hiding this comment.
We've not done that in other dialogs that use AlertDialog, that I can see. Is that something we need to start enforcing?
There was a problem hiding this comment.
🤔
Maybe dialogs are the exception. Carry on without I guess 😄
...va/com/x8bit/bitwarden/ui/platform/components/dialog/BitwardenClientCertificateInfoDialog.kt
Show resolved
Hide resolved
SaintPatrck
force-pushed
the
PM-17531/tls-import-dialogs
branch
from
afdb921 to
2dc773d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4622 +/- ##
=======================================
Coverage 88.39% 88.39%
=======================================
Files 603 603
Lines 40905 40905
Branches 5755 5755
=======================================
Hits 36160 36160
Misses 2742 2742
Partials 2003 2003 ☔ View full report in Codecov by Sentry. |
| value = alias, | ||
| onValueChange = { alias = it }, | ||
| autoFocus = true, | ||
| cardStyle = CardStyle.Top(), |
There was a problem hiding this comment.
I believe this divider should have padding set to 0, since the content is a text field.
| value = password, | ||
| onValueChange = { password = it }, | ||
| cardStyle = CardStyle.Bottom, | ||
| textFieldTestTag = "AlertClientCertificatePasswordInputField", |
| modifier = Modifier.testTag("AlertContentText"), | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(8.dp)) |
There was a problem hiding this comment.
I believe the spacing between text and a card is 24dp, might be 16dp but I'm pretty sure it was 24dp
This commit introduces a new dialog for importing client certificates. The dialog prompts the user to enter the certificate password and alias.
SaintPatrck
force-pushed
the
PM-17531/tls-import-dialogs
branch
from
2dc773d to
aad081d
Compare
🎟️ Tracking
PM-17531
Relates to #4486
📔 Objective
This commit introduces a new dialog for importing client certificates. The dialog prompts the user to enter the certificate password and alias.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes