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

[PM-8217] New device notice email access UI #4400

Merged
merged 16 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,
Copy link
Contributor

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.

)
}
}

@Composable
private fun NewDeviceNoticeEmailAccessContent(
email: String,
isEmailAccessEnabled: Boolean,
onEmailAccessToggleChanged: (Boolean) -> Unit,
onContinueClick: () -> Unit,
modifier: Modifier = Modifier,
) {
Column(
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 onEmailAccessToggleChanged and onContinueClick

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed since the same default Modifier is declared in the function signature.

)
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.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@Suppress("MaxLineLength")
@Composable
private fun HeaderContent(
modifier: Modifier = Modifier,
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(
Copy link
Collaborator

Choose a reason for hiding this comment

The 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")
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
Copy link
Contributor

Choose a reason for hiding this comment

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

โ›๏ธ isEnabled reads better. Not a blocker though. Just my OCD flaring up. ๐Ÿ˜†

}
Original file line number Diff line number Diff line change
Expand Up @@ -130,19 +130,22 @@ fun @receiver:StringRes Int.asText(vararg args: Any): Text = ResArgsText(this, a
* Create an [AnnotatedString] with highlighted parts.
* @param mainString the full string
* @param highlights parts of the mainString that will be highlighted
* @param highlightStyle the style to apply to the highlights
* @param mainStringStyle the style to apply to the mainString
* @param tag the tag that will be used for the annotation
*/
@Composable
fun createAnnotatedString(
mainString: String,
highlights: List<String>,
highlightStyle: SpanStyle = bitwardenClickableTextSpanStyle,
mainStringStyle: SpanStyle = bitwardenDefaultSpanStyle,
tag: String? = null,
): AnnotatedString {
return buildAnnotatedString {
append(mainString)
addStyle(
style = bitwardenDefaultSpanStyle,
style = mainStringStyle,
start = 0,
end = mainString.length,
)
Expand Down
Loading
Loading