feat(list-item): update component to version 0.3 adding custom contents, badge and tag as trailing elements

[paulinea]

No description provided.

[github-actions]

🟢 Netlify deploy for commit 6127a77 succeeded

Deploy preview: https://6ad7350ac985d4bda50345d5--ouds-android.netlify.app
Deploy log: https://app.netlify.com/projects/ouds-android/deploys/6ad7350ac985d4bda50345d5

[florentmaitre]

Adding new parameters here instead of the last position might break source compatibility. This is highly unlikely because this would only happen if a user called this method without any parameter name.

Until now we added new parameters at the end to ensure we don't have any source compatibility issue, but we can decide to do differently if you want.

Here is what the official guidelines says:

Since every compose is a function, the following rules apply to the component API changes:

• Parameters of the functions MUST NOT be removed.
• Newly added parameter to existing functions MUST have default expressions.
• New parameters MAY be added as a last parameter, or second to last in cases of trailing lambdas.
• The developer might decide to put the new parameter closer to other parameters that are semantically close to a new one. Keep in mind that this might break source compatibility if the user uses the component without named parameters.

https://android.googlesource.com/platform/frameworks/support/+/androidx-main/compose/docs/compose-component-api-guidelines.md#evolution-of-the-component-apis

Same comment for OudsCardItem.

[florentmaitre]

It's a detail and I am not even sure you need to change this because it does not have any impact and will be updated by the prepareRelease task, but we should write @IntroducedAt("2.3.0-Unreleased").

[florentmaitre]

Min height is also applied to the Row below (this is also the case in Figma). We can keep it to match Figma but it should have no effect on the Column.

[paulinea]

I think we always had this case for another component but I don't remember which one and what we decided.

[florentmaitre]

As this composable does not have Box parameter (such as contentAlignment or propagateMinConstraints), what do you think or renaming it to CustomContent (or ListItemContent)?

[florentmaitre]

In OudsTagAsset and OudsBulletListUnorderedAsset which also are OudsPolymorphicComponentContent, we only declare a single ExtraParameters class that contains all properties. Some properties are sometimes useless (depending on the cases) but this simplifies the code here and where we call the PolymorphicContent method.

Then the following code:

trailing?.let {
    when (trailing) {
        is OudsListItemLeadingTrailing.Badge -> trailing.PolymorphicContent(
            extraParameters = OudsListItemLeadingTrailing.Badge.ExtraParameters(state = state)
        )
        is OudsListItemLeadingTrailing.Content -> trailing.PolymorphicContent(
            extraParameters = OudsListItemLeadingTrailing.Content.ExtraParameters(state = state)
        )
        is OudsListItemLeadingTrailing.Icon -> trailing.PolymorphicContent(
            extraParameters = OudsListItemLeadingTrailing.Icon.ExtraParameters(state = state)
        )
        is OudsListItemLeadingTrailing.Image -> trailing.PolymorphicContent()
        is OudsListItemLeadingTrailing.Tag -> trailing.PolymorphicContent(
            extraParameters = OudsListItemLeadingTrailing.Tag.ExtraParameters(state = state)
        )
        is OudsListItemLeadingTrailing.Text -> {
            trailing.PolymorphicContent(
                extraParameters = OudsListItemLeadingTrailing.Text.ExtraParameters(
                    state = state,
                    verticalAlignment = verticalAlignment,
                    size = size
                )
            )
        }
    }
}

could be simplified to this:

trailing?.PolymorphicContent(
    extraParameters = OudsListItemLeadingTrailing.ExtraParameters(
        state = state,
        verticalAlignment = verticalAlignment,
        size = size
    )
)

However, I am not sure because the dump does not seems to change but I wonder if this might break binary compatibility because for instance OudsListItemBadge which inherits from OudsComponentContent<OudsListItemLeadingTrailing.Badge.ExtraParameters> will now inherit from OudsComponentContent<OudsListItemLeadingTrailing.ExtraParameters>.

We can discuss that point after everything else is OK if you want, because I don't know if it's worth it.

[florentmaitre]

This is not needed because the badge will inherit the enabled state of the list item.

Same comment for the tag.

If we fix this, this will also impact the code snippet.

[florentmaitre]

I was looking for a way to factorize calls to CustomContent and I found this solution:

val baseLabelContent: @Composable OudsListItemScope.() -> Unit = requireNotNull(getContentOrNull(true))

or this one:

fun getContentOrNull(contentActivated: Boolean): @Composable (OudsListItemScope.() -> Unit)? = if (contentActivated) getContent() else null

fun getContent(): @Composable (OudsListItemScope.() -> Unit) = { CustomContent(state = state, modifier = Modifier.fillMaxWidth()) }

where getContent would replace baseLabelContent.

By the way, maybe getBaseListItemContent would be more appropriate because this is a top level function.

[florentmaitre]

This should be:

val enabled = state != OudsListItemState.Disabled

Also, that's very minor, but this method could be private.

[florentmaitre]

For other components, we chose to disabled the entries rather than hiding them. For instance for the FAB:

val enabledLayouts: List<Layout>
    get() = when (size) {
        Size.Small,
        Size.Large -> listOf(Layout.IconOnly)
        Size.Medium -> Layout.entries
    }

I don't know if we must follow the same pattern here or if we should hide them as you did.

[florentmaitre]

What do you think about renaming Example to Settings?

Also, there is a typo here, this should be Example instead of Exemple.