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.

Sign up for GitHub

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

Android-13783 Improve Callout buttons margin #304

Merged
merged 4 commits into from
Sep 26, 2023
Merged

Android-13783 Improve Callout buttons margin #304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c9a4a8d
ANDROID-13783 Update. Improve margins for Callout component
haynlo Sep 25, 2023
e6c33ea
ANDROID-13783 Update. Make private the implementation of ListRowItem
haynlo Sep 25, 2023
7de8c99
ANDROID-13783 Update. Align XML Catalog option names with the Compose…
haynlo Sep 25, 2023
06a7777
ANDROID-13783 Update. Retro-compatibility for Button padding
haynlo Sep 26, 2023
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
16 changes: 8 additions & 8 deletions ...main/java/com/telefonica/mistica/catalog/ui/classic/components/CalloutsCatalogFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class CalloutsCatalogFragment : Fragment() {
CalloutButtonsConfig.values().map { it.name },
)
)
setText(CalloutButtonsConfig.BUTTONS_CONFIG_PRIMARY.name)
setText(CalloutButtonsConfig.PRIMARY.name)
}
view.findViewById<Button>(R.id.update_button).setOnClickListener {
updateCalloutView(view)
Expand Down Expand Up @@ -87,12 +87,12 @@ class CalloutsCatalogFragment : Fragment() {
}

private enum class CalloutButtonsConfig(@CalloutView.ButtonsConfig val buttonsConfig: Int) {
BUTTONS_CONFIG_NONE(CalloutView.BUTTONS_CONFIG_NONE),
BUTTONS_CONFIG_PRIMARY(CalloutView.BUTTONS_CONFIG_PRIMARY),
BUTTONS_CONFIG_PRIMARY_LINK(CalloutView.BUTTONS_CONFIG_PRIMARY_LINK),
BUTTONS_CONFIG_PRIMARY_SECONDARY(CalloutView.BUTTONS_CONFIG_PRIMARY_SECONDARY),
BUTTONS_CONFIG_SECONDARY(CalloutView.BUTTONS_CONFIG_SECONDARY),
BUTTONS_CONFIG_SECONDARY_LINK(CalloutView.BUTTONS_CONFIG_SECONDARY_LINK),
BUTTONS_CONFIG_LINK(CalloutView.BUTTONS_CONFIG_LINK),
NONE(CalloutView.BUTTONS_CONFIG_NONE),
PRIMARY(CalloutView.BUTTONS_CONFIG_PRIMARY),
PRIMARY_AND_LINK(CalloutView.BUTTONS_CONFIG_PRIMARY_LINK),
PRIMARY_AND_SECONDARY(CalloutView.BUTTONS_CONFIG_PRIMARY_SECONDARY),
SECONDARY(CalloutView.BUTTONS_CONFIG_SECONDARY),
SECONDARY_AND_LINK(CalloutView.BUTTONS_CONFIG_SECONDARY_LINK),
LINK(CalloutView.BUTTONS_CONFIG_LINK),
Comment on lines +90 to +96
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Align DropDown option names with the catalog Compose ones in order to make it more readable.
https://github.com/Telefonica/mistica-android/blob/main/library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt#L170

}
}
5 changes: 3 additions & 2 deletions library/src/main/java/com/telefonica/mistica/compose/button/Button.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import com.telefonica.mistica.compose.theme.brand.MovistarBrand

private val iconSpacing = 10.dp
private val easing = CubicBezierEasing(0.77f, 0f, 0.175f, 1f)
private const val CHEVRON_ASPECT_RATIO = 8f/20f
private const val CHEVRON_ASPECT_RATIO = 8f / 20f

@Composable
fun Button(
Expand All @@ -62,6 +62,7 @@ fun Button(
enabled: Boolean = true,
@DrawableRes icon: Int? = null,
withChevron: Boolean = false,
invalidatePaddings: Boolean = false,
Copy link
Contributor

@jeprubio jeprubio Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding was that we were going to set a parameter for padding values with a default value and this way it would allow that value to be set from outside, but as that padding value depends on the type of the button that makes everything more messy. I guess this invalidatePaddings makes sense having that into account.

onClickListener: () -> Unit,
) {

Expand All @@ -84,7 +85,7 @@ fun Button(
}
.height(size.height)
.applyWidth(originalWidth),
contentPadding = PaddingValues(horizontal = size.contentPadding, vertical = 0.dp),
contentPadding = PaddingValues(horizontal = if (invalidatePaddings) 0.dp else size.horizontalPadding, vertical = 0.dp),
onClick = onClickListener,
enabled = enabled,
colors = style.buttonColors,
Expand Down
8 changes: 4 additions & 4 deletions library/src/main/java/com/telefonica/mistica/compose/button/ButtonSizes.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@ import androidx.compose.ui.unit.Dp
import androidx.compose.ui.unit.dp
import com.telefonica.mistica.compose.theme.MisticaTheme

internal class ButtonSizeConfig(
internal data class ButtonSizeConfig(
val textStyle: TextStyle,
val height: Dp,
val minWidth: Dp,
val iconSize: Dp,
val progressBarSize: Dp,
val progressBarStroke: Dp,
val contentPadding: Dp,
val horizontalPadding: Dp,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change it to a more accurate name.

)

@Composable
Expand All @@ -38,7 +38,7 @@ private fun getDefaultButtonSizeConfig() = ButtonSizeConfig(
iconSize = 24.dp,
progressBarSize = 20.dp,
progressBarStroke = 2.dp,
contentPadding = 16.dp
horizontalPadding = 16.dp
)

@Composable
Expand All @@ -49,5 +49,5 @@ private fun getSmallButtonSizeConfig() = ButtonSizeConfig(
iconSize = 16.dp,
progressBarSize = 16.dp,
progressBarStroke = 1.dp,
contentPadding = 12.dp
horizontalPadding = 12.dp
)
59 changes: 39 additions & 20 deletions library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.graphics.ColorFilter
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.unit.dp
import com.telefonica.mistica.R
import com.telefonica.mistica.compose.button.Button
Expand Down Expand Up @@ -88,27 +89,40 @@ fun Callout(
.padding(top = 16.dp),
) {
when (buttonConfig) {
CalloutButtonConfig.NONE -> { /* No buttons are shown */ }
CalloutButtonConfig.NONE -> { /* No buttons are shown */
}
CalloutButtonConfig.PRIMARY -> {
PrimaryButton(primaryButtonText, onPrimaryButtonClick)
PrimaryButton(text = primaryButtonText, onClick = onPrimaryButtonClick)
}
CalloutButtonConfig.PRIMARY_AND_LINK -> {
PrimaryButton(primaryButtonText, onPrimaryButtonClick)
LinkButton(linkText, onLinkClicked)
PrimaryButton(
text = primaryButtonText,
modifier = Modifier.padding(end = 16.dp),
onClick = onPrimaryButtonClick,
)
LinkButton(text = linkText, onClick = onLinkClicked)
}
CalloutButtonConfig.SECONDARY -> {
SecondaryButton(secondaryButtonText, onSecondaryButtonClick)
SecondaryButton(text = secondaryButtonText, onClick = onSecondaryButtonClick)
}
CalloutButtonConfig.PRIMARY_AND_SECONDARY -> {
PrimaryButton(primaryButtonText, onPrimaryButtonClick)
SecondaryButton(secondaryButtonText, onSecondaryButtonClick)
PrimaryButton(
text = primaryButtonText,
modifier = Modifier.padding(end = 16.dp),
onClick = onPrimaryButtonClick,
)
SecondaryButton(text = secondaryButtonText, onClick = onSecondaryButtonClick)
}
CalloutButtonConfig.SECONDARY_AND_LINK -> {
SecondaryButton(secondaryButtonText, onSecondaryButtonClick)
LinkButton(linkText, onLinkClicked)
SecondaryButton(
text = secondaryButtonText,
modifier = Modifier.padding(end = 16.dp),
onClick = onSecondaryButtonClick,
)
LinkButton(text = linkText, onClick = onLinkClicked)
}
CalloutButtonConfig.LINK -> {
LinkButton(linkText, onLinkClicked)
LinkButton(text = linkText, onClick = onLinkClicked)
}
}
}
Expand All @@ -117,10 +131,9 @@ fun Callout(
if (dismissable) {
Image(
modifier = Modifier
.padding(start = 16.dp)
Copy link
Contributor Author

@haynlo haynlo Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was other of the reasons... if close image takes 16dp of start padding, then the entire column will have this padding and therefore affecting to the link button space to be displayed.

In addition, I have verified that this Image don't need an additional start padding it will be space enough between the title and the image.

.clickable { onDismiss?.invoke() },
painter = painterResource(id = R.drawable.icn_cross),
contentDescription = null
contentDescription = stringResource(id = R.string.close_button_content_description)
)
}
}
Expand All @@ -130,39 +143,45 @@ fun Callout(
@Composable
private fun PrimaryButton(
text: String?,
onClick: (() -> Unit)? = null
modifier: Modifier = Modifier,
onClick: (() -> Unit)? = null,
) {
CalloutButton(text, onClick, ButtonStyle.PRIMARY_SMALL)
CalloutButton(text = text, onClick = onClick, style = ButtonStyle.PRIMARY_SMALL, modifier = modifier)
}

@Composable
private fun SecondaryButton(
text: String?,
onClick: (() -> Unit)? = null
modifier: Modifier = Modifier,
onClick: (() -> Unit)? = null,
) {
CalloutButton(text, onClick, ButtonStyle.SECONDARY_SMALL)
CalloutButton(text, onClick, ButtonStyle.SECONDARY_SMALL, modifier)
}

@Composable
private fun LinkButton(
text: String?,
onClick: (() -> Unit)? = null
modifier: Modifier = Modifier,
onClick: (() -> Unit)? = null,
) {
CalloutButton(text, onClick, ButtonStyle.LINK)
CalloutButton(text, onClick, ButtonStyle.LINK, modifier, true)
}

@Composable
private fun CalloutButton(
text: String?,
onClick: (() -> Unit)?,
style: ButtonStyle,
modifier: Modifier = Modifier,
invalidatePaddings: Boolean = false,
) {
text?.let {
Button(
modifier = Modifier.padding(end = 16.dp),
modifier = modifier,
text = text,
buttonStyle = style,
onClickListener = { onClick?.invoke() }
onClickListener = { onClick?.invoke() },
invalidatePaddings = invalidatePaddings,
)
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/src/main/java/com/telefonica/mistica/compose/list/ListRowItem.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ fun ListRowItem(

@ExperimentalMaterialApi
@Composable
fun ListRowItemImp(
private fun ListRowItemImp(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Component implementations should be private.
I will notify other people to check possible breaking changes with that (Latch, Smart-Wifi and Novum have been checked and don't have any).

modifier: Modifier = Modifier,
icon: @Composable (() -> Unit)? = null,
title: String? = null,
Expand Down