[PM-8217] New device notice email access UI #4400
Conversation
# Conflicts: # app/src/main/res/values/strings.xml
andrebispo5
requested review from
brian-livefront,
david-livefront,
dseverns-livefront,
ahaisting-livefront and
a team
as code owners
|
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4400 +/- ##
=========================================
+ Coverage 0 88.92% +88.92%
=========================================
Files 0 457 +457
Lines 0 39668 +39668
Branches 0 5610 +5610
=========================================
+ Hits 0 35276 +35276
- Misses 0 2432 +2432
- Partials 0 1960 +1960 ☔ View full report in Codecov by Sentry. |
| * The top level composable for the new device notice email access screen. | ||
| */ | ||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Suppress("LongMethod") |
There was a problem hiding this comment.
This does not look like the method is too long
| BitwardenScaffold( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .nestedScroll(scrollBehavior.nestedScrollConnection), |
There was a problem hiding this comment.
There is no toolbar, do we need this at all?
| private fun MainContent( | ||
| email: String, | ||
| isEmailAccessEnabled: Boolean = false, | ||
| onEmailAccessToggleChanged: (Boolean) -> Unit = {}, |
There was a problem hiding this comment.
Why do these have default values?
There was a problem hiding this comment.
Just left them after a layout rework. Removing them.
| isEmailAccessEnabled: Boolean = false, | ||
| onEmailAccessToggleChanged: (Boolean) -> Unit = {}, | ||
| ) { | ||
| Spacer(modifier = Modifier.size(24.dp)) |
There was a problem hiding this comment.
This can just be height, not size
| * The main content of the screen. | ||
| */ | ||
| @Composable | ||
| private fun MainContent( |
There was a problem hiding this comment.
Since this does not have a wrapping contain with a modifier, I believe the pattern requires us to scope the function to provide that extra context.
private fun ColumnScope.MainContent(| .padding(horizontal = 22.dp) | ||
| .fillMaxWidth() | ||
| .clip(RoundedCornerShape(size = 4.dp)) | ||
| .background(BitwardenTheme.colorScheme.background.secondary), |
There was a problem hiding this comment.
We should not be using custom stuff like this, we already have BitwardenContentCard to handle stuff like this.
There was a problem hiding this comment.
The BitwardenContentCard didn't seem to adapt to what I wanted.
There was a problem hiding this comment.
That seems to indicate that the UI is non-standard, I don't think we should be doing that.
|
@andrebispo5 Could you link the Figma files for this? This does not seem to match current design patterns used in the app. |
|
|
||
| private fun handleEmailAccessToggle(action: EmailAccessToggle) { | ||
| mutableStateFlow.update { | ||
| it.copy(isEmailAccessEnabled = action.newState) |
There was a problem hiding this comment.
What is the intended purpose of the switch?
Do we not send the code if they do not have access to their email?
There was a problem hiding this comment.
If the user has access to the email we can dismiss this flow.
…. Add modifier parameter for composables.
| Image( | ||
| painter = rememberVectorPainter(id = R.drawable.warning), | ||
| contentDescription = null, | ||
| modifier = Modifier.size(120.dp), | ||
| modifier = modifier.size(120.dp), |
There was a problem hiding this comment.
The Modifier should only ever be applied to the top level component, if you wanted to pass one in like this, you would want to wrap it in a column.
| onEmailAccessToggleChanged: (Boolean) -> Unit = {}, | ||
| onContinueClick: () -> Unit = {}, | ||
| ) { | ||
| Column( |
There was a problem hiding this comment.
Since you went the route of adding a top-level column, be sure to pass in a modifier.
We also do not need default params for the onEmailAccessToggleChanged and onContinueClick
| ) { | ||
| Column( | ||
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| modifier = modifier.padding(horizontal = 24.dp), |
There was a problem hiding this comment.
Why is this using a non-standard amount of padding?
Can we discuss this with design because this screen is going to be difficult to maintain since it uses so many non-standard design patterns.
|
@david-livefront Layout was updated to reflect the new designs. |
…warden/android into pm-8217/new-device-notice-ui
| /** | ||
| * Header content containing the warning icon and title. | ||
| */ | ||
|
|
| } | ||
| }, | ||
| onContinueClick = { viewModel.trySendAction(ContinueClick) }, | ||
| modifier = Modifier, |
There was a problem hiding this comment.
This isn't necessary since there's a default Modifier declared.
| /** | ||
| * User tapped the email access toggle. | ||
| */ | ||
| data class EmailAccessToggle(val newState: Boolean) : NewDeviceNoticeEmailAccessAction() |
There was a problem hiding this comment.
⛏️ isEnabled reads better. Not a blocker though. Just my OCD flaring up. 😆
| /** | ||
| * Manages application state for the new device notice email access screen. | ||
| */ | ||
| @Suppress("TooManyFunctions") |
There was a problem hiding this comment.
I don't think this is needed. There are only a handful of functions. 🤔
| email = email, | ||
| isEmailAccessEnabled = isEmailAccessEnabled, | ||
| onEmailAccessToggleChanged = onEmailAccessToggleChanged, | ||
| modifier = Modifier, |
There was a problem hiding this comment.
Not needed since the same default Modifier is declared in the function signature.
| private fun HeaderContent( | ||
| modifier: Modifier = Modifier, | ||
| ) { |
There was a problem hiding this comment.
🎨 If you scope this it will inherit the modifier and other context from its parent Column scope. That would mean you don't need a wrapping Column within HeaderContent to set the modifier and horizontalAlignment. It would look like...
fun NewDeviceNoticeEmailAccessContent(...) {
Column(
horizontalAlignment = Alignment.CenterHorizontally,
modifier = modifier
.standardHorizontalMargin()
.fillMaxSize()
.verticalScroll(state = rememberScrollState()),
) {
Spacer(...)
HeaderContent()
// other components
}
}
private fun ColumnScope.HeaderContent() {
Image(...)
Spacer(...)
// other components
} This works because HeaderContent is using the same modifier and horizontalAlignment from the parent Column scope. It doesn't work quite as cleanly for MainContent because you're not using the same horizontal alignment as its parent Column scope.
Consider this more of a FYI than an actual change request since there's nothing technically wrong with the way you've done it.
There was a problem hiding this comment.
Oh I got it and thanks for the explanation, it makes sense specially the part that it's the same as the parent. I'll change it since it clears a lot of unnecessary code lines 👍
🎟️ Tracking
PM-8217 Add notice to users with no 2FA
📔 Objective
Add first screen for the notice to users with no 2FA
📸 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