Android-13783 Improve Callout buttons margin #304

haynlo · 2023-09-25T16:09:13Z

🥅 What's the goal?

Improve Callout buttons margin. Check 'How can I test this?' section to see the main differences.

🚧 How do we do it?

Improve Margins definition for Callout compose Component
Remove link buttons horizontal margin

☑️ Checks

I updated the documentation, including readmes and wikis. If this is a breaking change, update UPGRADING.md to inform users how to proceed. If no updates are necessary, indicate so.
Tested with dark mode.
Tested with API 23.

🧪 How can I test this?

Before	After	Touch target size

github-actions · 2023-09-25T16:16:29Z

📱 New catalog for testing generated: Download

… Catalog ones.

haynlo · 2023-09-25T16:23:42Z

...rc/main/java/com/telefonica/mistica/catalog/ui/classic/components/CalloutsCatalogFragment.kt

+        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),


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

haynlo · 2023-09-25T16:24:11Z

library/src/main/java/com/telefonica/mistica/compose/button/ButtonSizes.kt

    val textStyle: TextStyle,
    val height: Dp,
    val minWidth: Dp,
    val iconSize: Dp,
    val progressBarSize: Dp,
    val progressBarStroke: Dp,
-    val contentPadding: Dp,
+    val horizontalPadding: Dp,


Change it to a more accurate name.

haynlo · 2023-09-25T16:26:27Z

library/src/main/java/com/telefonica/mistica/compose/button/ButtonSizes.kt

        ButtonStyle.LINK,
        ButtonStyle.LINK_INVERSE,
-        -> getSmallButtonSizeConfig()
+        -> getSmallButtonSizeConfig().copy(horizontalPadding = 0.dp)


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.

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

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

github-actions · 2023-09-25T16:26:37Z

📱 New catalog for testing generated: Download

haynlo · 2023-09-25T16:31:37Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

@@ -117,10 +131,9 @@ fun Callout(
            if (dismissable) {
                Image(
                    modifier = Modifier
-                        .padding(start = 16.dp)


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.

haynlo · 2023-09-25T16:34:17Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

-                            PrimaryButton(primaryButtonText, onPrimaryButtonClick)
-                            SecondaryButton(secondaryButtonText, onSecondaryButtonClick)
+                            PrimaryButton(
+                                modifier = Modifier.padding(end = 16.dp),


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.

haynlo · 2023-09-25T16:37:01Z

library/src/main/java/com/telefonica/mistica/compose/list/ListRowItem.kt

@@ -109,7 +109,7 @@ fun ListRowItem(

 @ExperimentalMaterialApi
 @Composable
-fun ListRowItemImp(
+private fun ListRowItemImp(


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

jeprubio · 2023-09-26T07:15:21Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

+    modifier: Modifier = Modifier,
    text: String?,
-    onClick: (() -> Unit)? = null
+    onClick: (() -> Unit)? = null,


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.

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 :)

jeprubio · 2023-09-26T07:19:55Z

library/src/main/java/com/telefonica/mistica/compose/button/ButtonSizes.kt

        ButtonStyle.LINK,
        ButtonStyle.LINK_INVERSE,
-        -> getSmallButtonSizeConfig()
+        -> getSmallButtonSizeConfig().copy(horizontalPadding = 0.dp)


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

jeprubio

Nice

dagonco

Nice @haynlo! 😇

github-actions · 2023-09-26T11:41:15Z

📱 New catalog for testing generated: Download

jeprubio

LGTM. Thanks!

jeprubio · 2023-09-26T11:43:12Z

library/src/main/java/com/telefonica/mistica/compose/button/Button.kt

@@ -62,6 +62,7 @@ fun Button(
    enabled: Boolean = true,
    @DrawableRes icon: Int? = null,
    withChevron: Boolean = false,
+    invalidatePaddings: Boolean = false,


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.

haynlo added 2 commits September 25, 2023 17:50

ANDROID-13783 Update. Improve margins for Callout component

c9a4a8d

ANDROID-13783 Update. Make private the implementation of ListRowItem

e6c33ea

ANDROID-13783 Update. Align XML Catalog option names with the Compose…

7de8c99

… Catalog ones.

haynlo commented Sep 25, 2023

View reviewed changes

haynlo requested review from a team, dagonco and jeprubio and removed request for a team September 25, 2023 16:37

jeprubio reviewed Sep 26, 2023

View reviewed changes

jeprubio approved these changes Sep 26, 2023

View reviewed changes

dagonco approved these changes Sep 26, 2023

View reviewed changes

ANDROID-13783 Update. Retro-compatibility for Button padding

06a7777

haynlo requested a review from jeprubio September 26, 2023 11:33

jeprubio approved these changes Sep 26, 2023

View reviewed changes

haynlo merged commit 641911e into main Sep 26, 2023
4 checks passed

haynlo deleted the haynlo/ANDROID-13783_callout_margins branch September 26, 2023 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android-13783 Improve Callout buttons margin #304

Android-13783 Improve Callout buttons margin #304

haynlo commented Sep 25, 2023 •

edited

Loading

github-actions bot commented Sep 25, 2023

haynlo Sep 25, 2023

haynlo Sep 25, 2023

haynlo Sep 25, 2023

jeprubio Sep 26, 2023

haynlo Sep 26, 2023

github-actions bot commented Sep 25, 2023

haynlo Sep 25, 2023 •

edited

Loading

haynlo Sep 25, 2023

haynlo Sep 25, 2023

jeprubio Sep 26, 2023

haynlo Sep 26, 2023

jeprubio Sep 26, 2023

jeprubio left a comment

dagonco left a comment

github-actions bot commented Sep 26, 2023

jeprubio left a comment

jeprubio Sep 26, 2023 •

edited

Loading

Android-13783 Improve Callout buttons margin #304

Android-13783 Improve Callout buttons margin #304

Conversation

haynlo commented Sep 25, 2023 • edited Loading

🥅 What's the goal?

🚧 How do we do it?

☑️ Checks

🧪 How can I test this?

github-actions bot commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2023

haynlo Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeprubio left a comment

Choose a reason for hiding this comment

dagonco left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2023

jeprubio left a comment

Choose a reason for hiding this comment

jeprubio Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

haynlo commented Sep 25, 2023 •

edited

Loading

haynlo Sep 25, 2023 •

edited

Loading

jeprubio Sep 26, 2023 •

edited

Loading