-
Notifications
You must be signed in to change notification settings - Fork 842
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
[PM-8217] New device notice email access UI #4400
Changes from 14 commits
5741333
cb26200
35c8b1c
0c74547
02803c3
58ab96f
ee645b5
c711348
804e3f4
00b0680
0eb63a2
b8fae5a
cb28215
b459632
157a0ca
0bb3a1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
package com.x8bit.bitwarden.ui.auth.feature.newdevicenotice | ||
|
||
import androidx.lifecycle.SavedStateHandle | ||
import androidx.navigation.NavController | ||
import androidx.navigation.NavGraphBuilder | ||
import androidx.navigation.NavOptions | ||
import androidx.navigation.NavType | ||
import androidx.navigation.navArgument | ||
import com.x8bit.bitwarden.data.platform.annotation.OmitFromCoverage | ||
import com.x8bit.bitwarden.ui.platform.base.util.composableWithSlideTransitions | ||
|
||
private const val EMAIL_ADDRESS = "email_address" | ||
private const val NEW_DEVICE_NOTICE_PREFIX = "new_device_notice" | ||
private const val NEW_DEVICE_NOTICE_EMAIL_ACCESS_ROUTE = | ||
"$NEW_DEVICE_NOTICE_PREFIX/{${EMAIL_ADDRESS}}" | ||
|
||
/** | ||
* Class to retrieve new device notice email access arguments from the [SavedStateHandle]. | ||
*/ | ||
@OmitFromCoverage | ||
data class NewDeviceNoticeEmailAccessArgs(val emailAddress: String) { | ||
constructor(savedStateHandle: SavedStateHandle) : this( | ||
checkNotNull(savedStateHandle[EMAIL_ADDRESS]) as String, | ||
) | ||
} | ||
|
||
/** | ||
* Navigate to the new device notice email access screen. | ||
*/ | ||
fun NavController.navigateToNewDeviceNoticeEmailAccess( | ||
emailAddress: String, | ||
navOptions: NavOptions? = null, | ||
) { | ||
this.navigate( | ||
route = "$NEW_DEVICE_NOTICE_PREFIX/$emailAddress", | ||
navOptions = navOptions, | ||
) | ||
} | ||
|
||
/** | ||
* Add the new device notice email access screen to the nav graph. | ||
*/ | ||
fun NavGraphBuilder.newDeviceNoticeEmailAccessDestination( | ||
onNavigateToTwoFactorOptions: () -> Unit, | ||
) { | ||
composableWithSlideTransitions( | ||
route = NEW_DEVICE_NOTICE_EMAIL_ACCESS_ROUTE, | ||
arguments = listOf( | ||
navArgument(EMAIL_ADDRESS) { type = NavType.StringType }, | ||
), | ||
) { | ||
NewDeviceNoticeEmailAccessScreen( | ||
onNavigateToTwoFactorOptions = onNavigateToTwoFactorOptions, | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,199 @@ | ||||
package com.x8bit.bitwarden.ui.auth.feature.newdevicenotice | ||||
|
||||
import androidx.compose.foundation.Image | ||||
import androidx.compose.foundation.layout.Column | ||||
import androidx.compose.foundation.layout.Spacer | ||||
import androidx.compose.foundation.layout.fillMaxSize | ||||
import androidx.compose.foundation.layout.height | ||||
import androidx.compose.foundation.layout.imePadding | ||||
import androidx.compose.foundation.layout.navigationBarsPadding | ||||
import androidx.compose.foundation.layout.size | ||||
import androidx.compose.foundation.rememberScrollState | ||||
import androidx.compose.foundation.verticalScroll | ||||
import androidx.compose.material3.Text | ||||
import androidx.compose.runtime.Composable | ||||
import androidx.compose.runtime.getValue | ||||
import androidx.compose.runtime.remember | ||||
import androidx.compose.ui.Alignment | ||||
import androidx.compose.ui.Modifier | ||||
import androidx.compose.ui.platform.testTag | ||||
import androidx.compose.ui.res.stringResource | ||||
import androidx.compose.ui.text.SpanStyle | ||||
import androidx.compose.ui.text.font.FontWeight | ||||
import androidx.compose.ui.text.style.TextAlign | ||||
import androidx.compose.ui.tooling.preview.PreviewScreenSizes | ||||
import androidx.compose.ui.unit.dp | ||||
import androidx.hilt.navigation.compose.hiltViewModel | ||||
import androidx.lifecycle.compose.collectAsStateWithLifecycle | ||||
import com.x8bit.bitwarden.R | ||||
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeEmailAccessAction.ContinueClick | ||||
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeEmailAccessAction.EmailAccessToggle | ||||
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeEmailAccessEvent.NavigateToTwoFactorOptions | ||||
import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect | ||||
import com.x8bit.bitwarden.ui.platform.base.util.createAnnotatedString | ||||
import com.x8bit.bitwarden.ui.platform.base.util.standardHorizontalMargin | ||||
import com.x8bit.bitwarden.ui.platform.components.button.BitwardenFilledButton | ||||
import com.x8bit.bitwarden.ui.platform.components.scaffold.BitwardenScaffold | ||||
import com.x8bit.bitwarden.ui.platform.components.toggle.BitwardenSwitch | ||||
import com.x8bit.bitwarden.ui.platform.components.util.rememberVectorPainter | ||||
import com.x8bit.bitwarden.ui.platform.theme.BitwardenTheme | ||||
|
||||
/** | ||||
* The top level composable for the new device notice email access screen. | ||||
*/ | ||||
@Composable | ||||
fun NewDeviceNoticeEmailAccessScreen( | ||||
onNavigateToTwoFactorOptions: () -> Unit, | ||||
viewModel: NewDeviceNoticeEmailAccessViewModel = hiltViewModel(), | ||||
) { | ||||
val state by viewModel.stateFlow.collectAsStateWithLifecycle() | ||||
EventsEffect(viewModel = viewModel) { event -> | ||||
when (event) { | ||||
NavigateToTwoFactorOptions -> onNavigateToTwoFactorOptions() | ||||
} | ||||
} | ||||
|
||||
BitwardenScaffold { | ||||
NewDeviceNoticeEmailAccessContent( | ||||
email = state.email, | ||||
isEmailAccessEnabled = state.isEmailAccessEnabled, | ||||
onEmailAccessToggleChanged = remember(viewModel) { | ||||
{ newState -> | ||||
viewModel.trySendAction(EmailAccessToggle(newState = newState)) | ||||
} | ||||
}, | ||||
onContinueClick = { viewModel.trySendAction(ContinueClick) }, | ||||
modifier = Modifier, | ||||
) | ||||
} | ||||
} | ||||
|
||||
@Composable | ||||
private fun NewDeviceNoticeEmailAccessContent( | ||||
email: String, | ||||
isEmailAccessEnabled: Boolean, | ||||
onEmailAccessToggleChanged: (Boolean) -> Unit, | ||||
onContinueClick: () -> Unit, | ||||
modifier: Modifier = Modifier, | ||||
) { | ||||
Column( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||
horizontalAlignment = Alignment.CenterHorizontally, | ||||
modifier = modifier | ||||
.standardHorizontalMargin() | ||||
.fillMaxSize() | ||||
.verticalScroll(state = rememberScrollState()), | ||||
) { | ||||
Spacer(modifier = Modifier.height(104.dp)) | ||||
HeaderContent() | ||||
Spacer(modifier = Modifier.height(24.dp)) | ||||
MainContent( | ||||
email = email, | ||||
isEmailAccessEnabled = isEmailAccessEnabled, | ||||
onEmailAccessToggleChanged = onEmailAccessToggleChanged, | ||||
modifier = Modifier, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed since the same default |
||||
) | ||||
Spacer(modifier = Modifier.height(24.dp)) | ||||
BitwardenFilledButton( | ||||
label = stringResource(R.string.continue_text), | ||||
onClick = onContinueClick, | ||||
modifier = Modifier | ||||
.fillMaxSize() | ||||
.imePadding(), | ||||
) | ||||
Spacer(modifier = Modifier.navigationBarsPadding()) | ||||
} | ||||
} | ||||
|
||||
/** | ||||
* Header content containing the warning icon and title. | ||||
*/ | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
@Suppress("MaxLineLength") | ||||
@Composable | ||||
private fun HeaderContent( | ||||
modifier: Modifier = Modifier, | ||||
) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ If you scope this it will inherit the 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 ๐ |
||||
Column( | ||||
horizontalAlignment = Alignment.CenterHorizontally, | ||||
modifier = modifier, | ||||
) { | ||||
Image( | ||||
painter = rememberVectorPainter(id = R.drawable.warning), | ||||
contentDescription = null, | ||||
modifier = Modifier.size(120.dp), | ||||
) | ||||
Spacer(modifier = Modifier.height(24.dp)) | ||||
Text( | ||||
text = stringResource(R.string.important_notice), | ||||
style = BitwardenTheme.typography.titleMedium, | ||||
color = BitwardenTheme.colorScheme.text.primary, | ||||
textAlign = TextAlign.Center, | ||||
) | ||||
Spacer(modifier = Modifier.height(12.dp)) | ||||
Text( | ||||
text = stringResource( | ||||
R.string.bitwarden_will_soon_send_a_code_to_your_account_email_to_verify_logins_from_new_devices_in_february, | ||||
), | ||||
style = BitwardenTheme.typography.bodyMedium, | ||||
color = BitwardenTheme.colorScheme.text.primary, | ||||
textAlign = TextAlign.Center, | ||||
) | ||||
} | ||||
} | ||||
|
||||
/** | ||||
* The main content of the screen. | ||||
*/ | ||||
@Composable | ||||
private fun MainContent( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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( |
||||
email: String, | ||||
isEmailAccessEnabled: Boolean, | ||||
onEmailAccessToggleChanged: (Boolean) -> Unit, | ||||
modifier: Modifier = Modifier, | ||||
) { | ||||
Column( | ||||
modifier = modifier, | ||||
) { | ||||
Text( | ||||
text = createAnnotatedString( | ||||
mainString = stringResource( | ||||
R.string.do_you_have_reliable_access_to_your_email, | ||||
email, | ||||
), | ||||
mainStringStyle = SpanStyle( | ||||
color = BitwardenTheme.colorScheme.text.primary, | ||||
fontSize = BitwardenTheme.typography.bodyLarge.fontSize, | ||||
fontWeight = FontWeight.Normal, | ||||
), | ||||
highlights = listOf(email), | ||||
highlightStyle = SpanStyle( | ||||
color = BitwardenTheme.colorScheme.text.primary, | ||||
fontSize = BitwardenTheme.typography.bodyLarge.fontSize, | ||||
fontWeight = FontWeight.Bold, | ||||
), | ||||
), | ||||
) | ||||
Column { | ||||
BitwardenSwitch( | ||||
label = stringResource(id = R.string.yes_i_can_reliably_access_my_email), | ||||
isChecked = isEmailAccessEnabled, | ||||
onCheckedChange = onEmailAccessToggleChanged, | ||||
modifier = Modifier | ||||
.testTag("EmailAccessToggle"), | ||||
) | ||||
} | ||||
} | ||||
} | ||||
|
||||
@PreviewScreenSizes | ||||
@Composable | ||||
private fun NewDeviceNoticeEmailAccessScreen_preview() { | ||||
BitwardenTheme { | ||||
NewDeviceNoticeEmailAccessContent( | ||||
email = "test@bitwarden.com", | ||||
isEmailAccessEnabled = true, | ||||
onEmailAccessToggleChanged = {}, | ||||
onContinueClick = {}, | ||||
) | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
package com.x8bit.bitwarden.ui.auth.feature.newdevicenotice | ||
|
||
import android.os.Parcelable | ||
import androidx.lifecycle.SavedStateHandle | ||
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeEmailAccessAction.ContinueClick | ||
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeEmailAccessAction.EmailAccessToggle | ||
import com.x8bit.bitwarden.ui.auth.feature.newdevicenotice.NewDeviceNoticeEmailAccessEvent.NavigateToTwoFactorOptions | ||
import com.x8bit.bitwarden.ui.platform.base.BaseViewModel | ||
import dagger.hilt.android.lifecycle.HiltViewModel | ||
import kotlinx.coroutines.flow.update | ||
import kotlinx.parcelize.Parcelize | ||
import javax.inject.Inject | ||
|
||
private const val KEY_STATE = "state" | ||
|
||
/** | ||
* Manages application state for the new device notice email access screen. | ||
*/ | ||
@Suppress("TooManyFunctions") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is needed. There are only a handful of functions. ๐ค |
||
@HiltViewModel | ||
class NewDeviceNoticeEmailAccessViewModel @Inject constructor( | ||
savedStateHandle: SavedStateHandle, | ||
) : BaseViewModel< | ||
NewDeviceNoticeEmailAccessState, | ||
NewDeviceNoticeEmailAccessEvent, | ||
NewDeviceNoticeEmailAccessAction, | ||
>( | ||
initialState = savedStateHandle[KEY_STATE] | ||
?: NewDeviceNoticeEmailAccessState( | ||
email = NewDeviceNoticeEmailAccessArgs(savedStateHandle).emailAddress, | ||
isEmailAccessEnabled = false, | ||
), | ||
) { | ||
override fun handleAction(action: NewDeviceNoticeEmailAccessAction) { | ||
when (action) { | ||
ContinueClick -> handleContinueClick() | ||
is EmailAccessToggle -> handleEmailAccessToggle(action) | ||
} | ||
} | ||
|
||
private fun handleContinueClick() { | ||
sendEvent(NavigateToTwoFactorOptions) | ||
} | ||
|
||
private fun handleEmailAccessToggle(action: EmailAccessToggle) { | ||
mutableStateFlow.update { | ||
it.copy(isEmailAccessEnabled = action.newState) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the user has access to the email we can dismiss this flow. |
||
} | ||
} | ||
} | ||
|
||
/** | ||
* Models state of the new device notice email access screen. | ||
*/ | ||
@Parcelize | ||
data class NewDeviceNoticeEmailAccessState( | ||
val email: String, | ||
val isEmailAccessEnabled: Boolean, | ||
) : Parcelable | ||
|
||
/** | ||
* Models events for the new device notice email access screen. | ||
*/ | ||
sealed class NewDeviceNoticeEmailAccessEvent { | ||
/** | ||
* Navigates to the Two Factor Options screen. | ||
*/ | ||
data object NavigateToTwoFactorOptions : NewDeviceNoticeEmailAccessEvent() | ||
} | ||
|
||
/** | ||
* Models actions for the new device notice email access screen. | ||
*/ | ||
sealed class NewDeviceNoticeEmailAccessAction { | ||
/** | ||
* User tapped the continue button. | ||
*/ | ||
data object ContinueClick : NewDeviceNoticeEmailAccessAction() | ||
|
||
/** | ||
* User tapped the email access toggle. | ||
*/ | ||
data class EmailAccessToggle(val newState: Boolean) : NewDeviceNoticeEmailAccessAction() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. โ๏ธ |
||
} |
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.
This isn't necessary since there's a default
Modifier
declared.