-
Notifications
You must be signed in to change notification settings - Fork 628
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
Update account item UI to support NME #8729
base: carlosmuvi/generic-pane-responses
Are you sure you want to change the base?
Update account item UI to support NME #8729
Conversation
f020998
to
062dae3
Compare
Diffuse output:
APK
|
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.
Are we sure about underlining the subtitles here? It looks rather jarring, and the container is already a selectable element.
@@ -63,8 +66,8 @@ internal fun AccountItem( | |||
networkedAccount: NetworkedAccount? = null, | |||
) { | |||
val view = LocalView.current | |||
// networked account's allowSelection takes precedence over the account's. | |||
val selectable = networkedAccount?.allowSelection ?: account.allowSelection | |||
val viewState = remember { getVisibilityState(account, networkedAccount) } |
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.
val viewState = remember { getVisibilityState(account, networkedAccount) } | |
val viewState = remember(account, networkedAccount) { | |
getVisibilityState(account, networkedAccount) | |
} |
if (SDK_INT >= M) view.performHapticFeedback(CONTEXT_CLICK) | ||
onAccountClicked(account) | ||
} | ||
.alpha(if (selectable) 1f else ContentAlpha.disabled) | ||
// Reduce | ||
.alpha(if (viewState == Enabled) 1f else VISUALLY_DISABLED_ALPHA) |
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.
Nit: We could add alpha
to ViewState
to simplify this.
@@ -180,6 +197,16 @@ private fun PartnerAccount.getFormattedBalance(): String? { | |||
} | |||
} | |||
|
|||
private enum class ViewState { |
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.
We should name this something else, as ViewState
is overly broad.
@@ -277,9 +277,18 @@ internal class LinkAccountPickerViewModel @AssistedInject constructor( | |||
eventTracker.track(event) | |||
} | |||
|
|||
fun onAccountClick(partnerAccount: PartnerAccount) { | |||
fun onAccountClick(partnerAccount: PartnerAccount) = viewModelScope.launch { |
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.
Doesn’t seem necessary to launch a coroutine here. Can you remove it?
Summary
Some UI and interaction changes related to the account view:
Motivation
📔 Support clickable + disabled accounts on LinkAccountPicker
🌐 BANKCON-11422
Testing
Screenshots
See generated screenshots
Changelog
No public changes, everything's behind a feature flag.