-
Notifications
You must be signed in to change notification settings - Fork 651
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
Refactor PaymentMethodRowButton to support embedded configurations #9596
base: master
Are you sure you want to change the base?
Conversation
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.
I think we should do this a different way (any of these work for me):
- We should separate the shell from the content in a pure refactor, no new functionality. Just enabling the inner content to be used by some different outer content later on.
- We should merge in the appearance API first, then make these changes.
I don't love having half baked code in the repo (TODOs aren't tracked anywhere!).
We also have a lot of opportunity to add screenshot tests here.
...verticalmode_PaymentMethodRowButtonCheckmarkScreenshotTest_testTailingContent[LargeFont].png
Show resolved
Hide resolved
...et.verticalmode_PaymentMethodRowButtonRadioScreenshotTest_testDisabledState[DefaultFont].png
Show resolved
Hide resolved
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.
These radio buttons look massive. Do they match the spec?
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.
They look equivalent to the ones in the design spec to me. I can make them smaller if desired. Do we have a figma with actual sizing?
@@ -31,47 +50,158 @@ internal fun PaymentMethodRowButton( | |||
subtitle: String?, | |||
onClick: () -> Unit, | |||
contentDescription: String? = null, | |||
modifier: Modifier = Modifier, | |||
modifier: Modifier = Modifier.fillMaxWidth(), |
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.
Should this move below? It feels like we'd always want this max width, not optionally?
8.dp | ||
} else { | ||
12.dp | ||
Row( |
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.
Why is the row needed? It looks like we can combine the modifiers with the card.
trailingContent: (@Composable RowScope.() -> Unit)? = null, | ||
) { | ||
Row( | ||
modifier = modifier.selectable( |
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.
It looks like we do this for every variant. Can we move this logic into the RowButtonContent
?
} | ||
} | ||
|
||
private fun String?.paddingValues(): Dp = if (this != null) 8.dp else 12.dp |
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 is a little deceptive. Maybe a rename to paddingValuesForSubtitle
would be better? (there's probably still a better name yet).
isEnabled = false, | ||
isSelected = false, | ||
iconContent = { | ||
Image( |
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.
I thought we'd have a generic inner content item, and that would be used for all types. This makes it feel like you would have to change 3 places if you wanted to add something new to the inner content.
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.
The spacing here looks wrong.
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.
Summary
Refactor
PaymentMethodRowButton
to support embedded configurations.Notes:
Motivation
MOBILESDK-2626
Testing