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 3 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

}
}
2 changes: 1 addition & 1 deletion 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 @@ -84,7 +84,7 @@ fun Button(
}
.height(size.height)
.applyWidth(originalWidth),
contentPadding = PaddingValues(horizontal = size.contentPadding, vertical = 0.dp),
contentPadding = PaddingValues(horizontal = size.horizontalPadding, vertical = 0.dp),
onClick = onClickListener,
enabled = enabled,
colors = style.buttonColors,
Expand Down
11 changes: 6 additions & 5 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 @@ -24,9 +24,10 @@ internal fun ButtonStyle.getButtonSizeCompose(): ButtonSizeConfig =
ButtonStyle.PRIMARY_SMALL,
ButtonStyle.SECONDARY_SMALL_INVERSE,
ButtonStyle.SECONDARY_SMALL,
-> getSmallButtonSizeConfig()
ButtonStyle.LINK,
ButtonStyle.LINK_INVERSE,
-> getSmallButtonSizeConfig()
-> getSmallButtonSizeConfig().copy(horizontalPadding = 0.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.

This is one of the reasons why the link button was not fully displayed.
We don't need to apply a horizontal padding because link buttons don't have a drawable background.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for removing always the horizontal padding of links? Are we sure this is the expected behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should be without padding the first time it was defined, but now I'm thinking that this change can lead to some kind of breaking change... Some people can be using this Button link keeping in mind this padding :(

As you can see in here, the link button doesn't need any extra padding. This is a wrong initial implementation, but you are right, maybe changing this to zero directly is a bit risky.. I'd better comment this in the Teams channel

else -> getDefaultButtonSizeConfig()
}

Expand All @@ -38,7 +39,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 +50,5 @@ private fun getSmallButtonSizeConfig() = ButtonSizeConfig(
iconSize = 16.dp,
progressBarSize = 16.dp,
progressBarStroke = 1.dp,
contentPadding = 12.dp
horizontalPadding = 12.dp
)
55 changes: 36 additions & 19 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(
modifier = Modifier.padding(end = 16.dp),
text = primaryButtonText,
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(
modifier = Modifier.padding(end = 16.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.

Another reason is that we were applying an end padding to all the buttons in Callout (primaries and secondaries).

Now we only apply this padding if there's another button to be displayed on the right, so the secondary button or secondary link button don't need to apply this padding and they can use this space to display de text completely.

text = primaryButtonText,
onClick = onPrimaryButtonClick
)
SecondaryButton(text = secondaryButtonText, onClick = onSecondaryButtonClick)
}
CalloutButtonConfig.SECONDARY_AND_LINK -> {
SecondaryButton(secondaryButtonText, onSecondaryButtonClick)
LinkButton(linkText, onLinkClicked)
SecondaryButton(
modifier = Modifier.padding(end = 16.dp),
text = secondaryButtonText,
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 @@ -129,37 +142,41 @@ fun Callout(

@Composable
private fun PrimaryButton(
modifier: Modifier = Modifier,
text: String?,
onClick: (() -> Unit)? = null
onClick: (() -> Unit)? = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

There are few places in this pr that the optional modifier is set as the first parameter, is this some kind of rule in this project or something? I'm used to set it as the first parameter which is optional but I prefer to set non optional parameters on top, like the Text Composable for example, I would expect to see the text parameter on top.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have a rule here. I always set the optional Modifier first for convenience but I don't have a strong opinion about that; your rule (non-optional first) makes more sense :)

) {
CalloutButton(text, onClick, ButtonStyle.PRIMARY_SMALL)
CalloutButton(modifier, text, onClick, ButtonStyle.PRIMARY_SMALL)
}

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

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

@Composable
private fun CalloutButton(
modifier: Modifier = Modifier,
text: String?,
onClick: (() -> Unit)?,
style: ButtonStyle,
) {
text?.let {
Button(
modifier = Modifier.padding(end = 16.dp),
modifier = modifier,
text = text,
buttonStyle = style,
onClickListener = { onClick?.invoke() }
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