[Compound] Platform components (Lists) #990

jmartinesp · 2023-07-27T15:53:32Z

Changes

Creates ListItem, ListSectionHeader and ListSupportingText composables. These new components will replace our current list items in a new PR, since this one is already large enough.

Why

Closes element-hq/compound#169

github-actions · 2023-07-28T08:04:31Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/QFcJi1

codecov · 2023-08-18T13:25:00Z

Codecov Report

Patch coverage: 64.60% and project coverage change: +0.05% 🎉

Comparison is base (6a4a4eb) 57.00% compared to head (1c714ab) 57.05%.
Report is 30 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #990      +/-   ##
===========================================
+ Coverage    57.00%   57.05%   +0.05%     
===========================================
  Files         1017     1019       +2     
  Lines        25822    26000     +178     
  Branches      5182     5262      +80     
===========================================
+ Hits         14720    14835     +115     
- Misses        8829     8862      +33     
- Partials      2273     2303      +30

Files Changed	Coverage Δ
...oid/libraries/designsystem/preview/PreviewGroup.kt	`0.00% <ø> (ø)`
...ibraries/designsystem/theme/components/ListItem.kt	`62.93% <62.93%> (ø)`
...aries/designsystem/theme/components/ListSection.kt	`67.74% <67.74%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ak screenshots naming

Added docs.

bmarty

LGTM, 2 minor remarks.

bmarty · 2023-08-18T15:03:08Z

...ystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/ListItem.kt

+    }
+    val leadingContentColor = when {
+        isPrimaryAction -> ElementTheme.colors.iconPrimary
+        isDestructive -> ElementTheme.colors.iconCriticalPrimary


Maybe test isDestructive first?

After thinking about it, I understand that parameters isPrimaryAction and isDestructive are probably exclusive (i.e. they cannot be both equal to true), but maybe enforce this by introducing an enum instead? If I am wrong, maybe add a preview with both parameter to true?

I implemented a style parameter to handle this in an enum way. Thanks!

bmarty · 2023-08-18T15:05:39Z

...ystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/ListItem.kt

+    isPrimaryAction: Boolean = false,
+    isDestructive: Boolean = false,
+    enabled: Boolean = true,
+    onClick: () -> Unit = { },


There is always a pb with having a default empty lambda, which is when the lambda stays empty, the click effect is still visible (since we invoke modifier.clickable(enabled = enabled, onClick = onClick) below). I think we may have ListItem with no click action, but if not, maybe remove the default parameter value. WDYT?

+1, if onClick is optional it must be nullable defaulting to null.

Done in 53ebf11.

- Switch `isPrimary` and `isDestructive` for a `style` sealed interface. - Made `onClick` nullable so there can be non-clickable list items. - Added `isCompact` parameter for customising the padding values. - Removed unnecessary parameters.

julioromano

Some API improvements suggestions

julioromano · 2023-08-21T06:38:46Z

...ystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/ListItem.kt

+    isPrimaryAction: Boolean = false,
+    isDestructive: Boolean = false,
+    enabled: Boolean = true,
+    onClick: () -> Unit = { },


+1, if onClick is optional it must be nullable defaulting to null.

julioromano · 2023-08-21T06:58:25Z

...ystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/ListItem.kt

+fun ListItem(
+    headlineContent: @Composable () -> Unit,
+    modifier: Modifier = Modifier,
+    overlineContent: @Composable (() -> Unit)? = null,


overlineContent seems unused, there's not even a preview of it and it's not in the figma. Perhaps we should think of removing it?

Done in 53ebf11

julioromano · 2023-08-21T07:05:45Z

...ystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/ListItem.kt

+ * @param enabled Whether the list item is enabled. When disabled, will change the color of the headline content and the leading content to use disabled tokens.
+ * @param onClick The callback to be called when the list item is clicked.
+ *
+ * **Note:** [isPrimaryAction], [isDestructive] and [enabled] should only be used in single line [ListItem] components,


There is no notion of "single line ListItem" as of yet. So this doc line leaves a lot to the dev's interpretation.

Perhaps we should remove isPrimaryAction and isDestructive from here, create another composable which only has a single line of content and also an enum param which is something like "StandardAction, PrimaryAction, DesctructiveAction".
This new composable could still be called ListItem (because the param list will be a lot different) or it could be called SingleLineListItem.

Actually, this sounds better than what was implemented on 53ebf11.

I still need to create a couple of PRs to replace usages of ListItems through the app and add Lists to dialogs. Do you mind if I add this change in one of this other PRs, or even a dedicated one for it?

Not a problem, but we can also leave it as is if it's not a problem to use "destructive" or "primary" style when there is also a supporting content.

bmarty · 2023-08-21T07:16:31Z

...ystem/src/main/kotlin/io/element/android/libraries/designsystem/theme/components/ListItem.kt

+
+    val setUpModifier = modifier
+        .then(if (isCompact) Modifier else Modifier.padding(vertical = 4.dp))
+        .clickable(enabled = onClick != null, onClick = onClick ?: {})


Maybe also take into account the enabled parameter? I do not know if we want to have a disabled ListItem still clickable. Maybe...

.clickable(enabled = enabled && onClick != null, onClick = onClick ?: {})

Yes, you're right. It depends on the implementation, but here enabled is up to us to apply, so it should be taken into account.

jmartinesp · 2023-08-21T07:20:59Z

I'll actually remove the isCompact parameter since it's not working as expected. Even when applied, the minHeight for the ListItem will always be 56.dp, while the Figma designs expects it to be 48.dp, which is not possible AFAICT.

sonarcloud · 2023-08-21T08:08:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

0.0% Coverage
0.0% Duplication

jmartinesp force-pushed the chore/jme/compound-platform-components-listitem branch 6 times, most recently from 5bc8a91 to 1372a89 Compare July 31, 2023 13:52

jmartinesp force-pushed the chore/jme/compound-platform-components-listitem branch 2 times, most recently from 455f865 to 3b65803 Compare August 18, 2023 12:59

jmartinesp marked this pull request as ready for review August 18, 2023 13:41

jmartinesp requested a review from a team as a code owner August 18, 2023 13:41

jmartinesp requested review from julioromano and removed request for a team August 18, 2023 13:41

jmartinesp and others added 15 commits August 18, 2023 15:53

Initial implementation of ListItem + previews

9fbebef

Suppress ModifierMissing warning in previews

cb59989

Use code regions instead of classes to wrap previews, so we don't bre…

ea0b143

…ak screenshots naming

Try to enforce a consistent line height style

60f8623

Fix InitialsAvatar alignment

e09b187

Fix more lint issues

d5b6dbd

Add ListSectionHeader and ListSupportingText components.

683c649

Added docs.

Fix rebase issues

3b09a9f

Update screenshots

99dfeba

Fix lint issues

64659a9

Try increasing heap size to prevent OOMs in screenshot tests

302efe4

Add changelog

a4a4ce4

Remove forcelineHeight function

887581e

Revert upgrading maxHeapSize to 2g

556059c

Fix typo in comment

0706c98

jmartinesp force-pushed the chore/jme/compound-platform-components-listitem branch from 14fa08a to 0706c98 Compare August 18, 2023 13:54

bmarty approved these changes Aug 18, 2023

View reviewed changes

Fixes:

53ebf11

- Switch `isPrimary` and `isDestructive` for a `style` sealed interface. - Made `onClick` nullable so there can be non-clickable list items. - Added `isCompact` parameter for customising the padding values. - Removed unnecessary parameters.

julioromano reviewed Aug 21, 2023

View reviewed changes

Update screenshots

b22ce5d

bmarty reviewed Aug 21, 2023

View reviewed changes

jmartinesp and others added 4 commits August 21, 2023 09:23

Disable click if enabled is false.

61eb913

Remove isCompact, as it's not working as expected.

ccd6223

Fix outdated docs and imports.

b7b00ed

Update screenshots

1c714ab

jmartinesp merged commit 0f77b45 into develop Aug 21, 2023
15 checks passed

jmartinesp deleted the chore/jme/compound-platform-components-listitem branch August 21, 2023 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compound] Platform components (Lists) #990

[Compound] Platform components (Lists) #990

jmartinesp commented Jul 27, 2023 •

edited

github-actions bot commented Jul 28, 2023 •

edited

codecov bot commented Aug 18, 2023 •

edited

bmarty left a comment

bmarty Aug 18, 2023

jmartinesp Aug 21, 2023

bmarty Aug 18, 2023

julioromano Aug 21, 2023

jmartinesp Aug 21, 2023

julioromano left a comment

julioromano Aug 21, 2023

julioromano Aug 21, 2023

jmartinesp Aug 21, 2023

julioromano Aug 21, 2023

jmartinesp Aug 21, 2023

julioromano Aug 21, 2023

bmarty Aug 21, 2023

jmartinesp Aug 21, 2023

jmartinesp commented Aug 21, 2023

sonarcloud bot commented Aug 21, 2023

[Compound] Platform components (Lists) #990

[Compound] Platform components (Lists) #990

Conversation

jmartinesp commented Jul 27, 2023 • edited

Changes

Why

github-actions bot commented Jul 28, 2023 • edited

codecov bot commented Aug 18, 2023 • edited

Codecov Report

bmarty left a 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

Choose a reason for hiding this comment

julioromano left a 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

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

jmartinesp commented Aug 21, 2023

sonarcloud bot commented Aug 21, 2023

jmartinesp commented Jul 27, 2023 •

edited

github-actions bot commented Jul 28, 2023 •

edited

codecov bot commented Aug 18, 2023 •

edited