Iterate on preference design #2922

bmarty · 2024-05-27T07:57:40Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Let Preference Composable use ListItem under the hood.

Motivation and context

Let the application globally follow the design.

(And preparatory work for #2912)

Screenshots / GIFs

See recorded once.

Tests

Navigate to the various preference screen and check that there is no issues.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

github-actions · 2024-05-27T08:09:15Z

📱 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/JLWBdw

Let PreferenceCheckbox use ListItem and add missing previews. Let PreferenceCategory use ListSectionHeader Let PreferenceSlide use ListItem Let PreferenceRow use ListItem Let PreferenceText use ListItem

Also improve height of View to let the test see the View.

codecov · 2024-05-27T12:11:51Z

Codecov Report

Attention: Patch coverage is 78.68852% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 75.15%. Comparing base (8f6190e) to head (414e2bb).
Report is 33 commits behind head on develop.

Files	Patch %	Lines
...omponents/preferences/components/PreferenceIcon.kt	55.55%	5 Missing and 3 partials ⚠️
...ignsystem/components/preferences/PreferenceText.kt	88.57%	0 Missing and 4 partials ⚠️
...references/impl/developer/DeveloperSettingsView.kt	57.14%	0 Missing and 3 partials ⚠️
...droid/features/roomdetails/impl/RoomDetailsView.kt	60.00%	0 Missing and 2 partials ⚠️
...signsystem/components/preferences/PreferenceRow.kt	71.42%	0 Missing and 2 partials ⚠️
...gnsystem/components/preferences/PreferenceSlide.kt	89.47%	0 Missing and 2 partials ⚠️
...lockscreen/impl/settings/LockScreenSettingsView.kt	0.00%	0 Missing and 1 partial ⚠️
...ystem/components/preferences/PreferenceCategory.kt	83.33%	0 Missing and 1 partial ⚠️
...ystem/components/preferences/PreferenceCheckbox.kt	88.88%	0 Missing and 1 partial ⚠️
...system/components/preferences/PreferenceDivider.kt	0.00%	0 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2922      +/-   ##
===========================================
- Coverage    75.23%   75.15%   -0.09%     
===========================================
  Files         1550     1549       -1     
  Lines        36954    36927      -27     
  Branches      7152     7170      +18     
===========================================
- Hits         27803    27752      -51     
- Misses        5415     5421       +6     
- Partials      3736     3754      +18

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

jmartinesp

Thanks for the work on this. To be honest I was going in the opposite direction, slowly replacing Preference* components with plain ListItem ones since that's what Compound gives us, so maybe we should first discuss which approach to take.

I'm not opposed to having EXA only wrappers but maybe they're not really needed in some cases, I'm not sure if this is one of them.

Besides that, I found a few things that could be improved since I think they'll affect paddings and internal margins.

jmartinesp · 2024-05-27T13:52:49Z

...t/android/features/preferences/impl/notifications/edit/EditDefaultNotificationSettingView.kt

@@ -83,7 +86,7 @@ fun EditDefaultNotificationSettingView(
            }
        }
        if (state.roomsWithUserDefinedMode.isNotEmpty()) {
-            PreferenceCategory(title = stringResource(id = R.string.screen_notification_settings_edit_custom_settings_section_title)) {
+            PreferenceCategory(title = stringResource(id = R.string.screen_notification_settings_edit_custom_settings_section_title),) {


Nit: unneeded comma here.

jmartinesp · 2024-05-27T14:02:59Z

...otlin/io/element/android/libraries/designsystem/components/preferences/PreferenceCategory.kt


 @Composable
 fun PreferenceCategory(
    modifier: Modifier = Modifier,
    title: String? = null,
-    showDivider: Boolean = true,
+    showTopDivider: Boolean = true,
+    showBottomDivider: Boolean = false,


showBottomDivider is never true, should we remove it? The ListSectionHeader component was built to always use only the top one ( which I admit is a bit awkward) so it shouldn't be needed.

jmartinesp · 2024-05-27T14:11:35Z

.../kotlin/io/element/android/libraries/designsystem/components/preferences/PreferenceSwitch.kt

            )
+        },
+        supportingContent = {


This should probably be supportingContent = subtitle?.let { ... }, I think otherwise the preference will add some spacing that's not really needed.

Yes, having an empty Composable does not seems to have an impact, but let's do what you're suggesting to be sure.

jmartinesp · 2024-05-27T14:13:11Z

...in/kotlin/io/element/android/libraries/designsystem/components/preferences/PreferenceText.kt

            Text(
                style = ElementTheme.typography.fontBodyLgRegular,
                text = title,
                color = tintColor ?: enabled.toEnabledColor(),
            )
+        },
+        supportingContent = {


Same here about the spacing, maybe inline the if here and add composable blocks to each branch, and null for the else.

jmartinesp · 2024-05-27T14:14:32Z

...in/kotlin/io/element/android/libraries/designsystem/components/preferences/PreferenceText.kt

-        }
-    }
+        },
+        trailingContent = ListItemContent.Custom {


Same about nullability here. If there's nothing to display it should be null, not an empty row.

jmartinesp · 2024-05-27T14:18:14Z

...o/element/android/libraries/designsystem/components/preferences/components/PreferenceIcon.kt

+    showIconAreaIfNoIcon: Boolean = false,
+): ListItemContent.Custom? {
+    return if (icon != null || iconResourceId != null || showIconAreaIfNoIcon) {
+        ListItemContent.Custom {


This isn't wrong, but maybe it'd be better to use the exiting ListItemContent.Icon when possible, maybe adding the badge option if needed, or a nullable version for the empty space. I'm not sure though, since that may complicate its usage.

Yes, there is room for improvement. "adding the badge option if needed" to ListItemContent.Icon should be my choice but I prefer to work on this later.

jmartinesp · 2024-05-27T14:26:21Z

I'm also quite confused about the coverage drop, the amount of covered lines should be pretty much the same 🫤 .

…ing to display.

bmarty · 2024-05-27T15:38:41Z

I'm also quite confused about the coverage drop, the amount of covered lines should be pretty much the same 🫤 .

Yes, let's ignore this for now...

bmarty · 2024-05-27T16:04:43Z

Thanks for the work on this. To be honest I was going in the opposite direction, slowly replacing Preference* components with plain ListItem ones since that's what Compound gives us, so maybe we should first discuss which approach to take.

I'm not opposed to having EXA only wrappers but maybe they're not really needed in some cases, I'm not sure if this is one of them.

Having Preference* Composable helps to have compact code, but yes, maybe one day we could get ride of them, and directly use ListItem. I consider those changes as a step to this direction.

jmartinesp

Thanks for the changes, LGTM!

sonarqubecloud · 2024-05-27T22:22:02Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 27, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 27, 2024

bmarty mentioned this pull request May 27, 2024

Iterate on preference design - DRAFT - STILL FIXING REGRESSION #2913

Closed

14 tasks

Let Preference composables use ListItem.

34f7819

Let PreferenceCheckbox use ListItem and add missing previews. Let PreferenceCategory use ListSectionHeader Let PreferenceSlide use ListItem Let PreferenceRow use ListItem Let PreferenceText use ListItem

bmarty force-pushed the feature/bma/iterateOnPreferenceDesign2 branch from 9f07601 to 34f7819 Compare May 27, 2024 10:19

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 27, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 27, 2024

ElementBot and others added 2 commits May 27, 2024 10:29

Update screenshots

8bf241b

Fix test by making the whole ListItem clickable.

b30076e

Also improve height of View to let the test see the View.

Fix formatting issue.

e74f82b

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 27, 2024

bmarty marked this pull request as ready for review May 27, 2024 12:38

bmarty requested a review from a team as a code owner May 27, 2024 12:38

bmarty requested review from jmartinesp and removed request for a team May 27, 2024 12:38

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 27, 2024

jmartinesp reviewed May 27, 2024

View reviewed changes

bmarty added 5 commits May 27, 2024 17:23

Remove trailing comma

6c11fb8

Remove unused parameter.

8eeb95e

Add preview without subtitle

cf05731

No Composable if subtitle is null

ef4cf42

trailingContent and supportingContent should be null if there is noth…

f743934

…ing to display.

bmarty added the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 27, 2024

github-actions bot removed the Record-Screenshots Runs the 'Record Screenshots' CI job and adds a commit with any new screenshots found. label May 27, 2024

Update screenshots

6339bb6

jmartinesp approved these changes May 27, 2024

View reviewed changes

bmarty added 2 commits May 28, 2024 00:11

supportingContent should be null if there is nothing to display.

7905767

UseLet

414e2bb

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 27, 2024

bmarty enabled auto-merge May 27, 2024 22:15

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 27, 2024

bmarty merged commit d0b2233 into develop May 27, 2024
24 of 25 checks passed

bmarty deleted the feature/bma/iterateOnPreferenceDesign2 branch May 27, 2024 22:36

Iterate on preference design #2922

Iterate on preference design #2922

Uh oh!

Conversation

bmarty commented May 27, 2024

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

github-actions bot commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jmartinesp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmartinesp commented May 27, 2024

Uh oh!

bmarty commented May 27, 2024

Uh oh!

bmarty commented May 27, 2024

Uh oh!

jmartinesp left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 27, 2024

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented May 27, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

jmartinesp left a comment •

edited

Loading