Add Compose list item family (one line, two line, settings) - #9249
Add Compose list item family (one line, two line, settings)#9249mikescamell wants to merge 6 commits into
Conversation
A standalone, label-agnostic pill for short status labels (e.g. Beta/New). Fixed DuckDuckGo-yellow palette in light and dark, matching the View DaxYellowPill; the caller supplies the (localised) label so the design system owns no product copy.
… to PreviewContainers PreviewSurface renders on a DaxSurface with no padding, for full-width components (list items) that carry their own internal padding and would look double-inset in the padded PreviewBox. The file is renamed to PreviewContainers now it holds more than a box.
mikescamell
changed the title
8ceb026 to
3174581
Compare
Adds DaxOneLineListItem, DaxTwoLineListItem and DaxSettingsListItem as thin public variants over an internal DaxListItem core with receiver-scoped leading/trailing slots, plus a DaxListItemDefaults object (PascalCase constants). Text params are runtime-stable: typed String / AnnotatedString public overloads funnel into a single AnnotatedString-typed core, so the composables stay skippable. Leading icons expose independent size and background axes with an optional tint; trailing content supports decorative or clickable icons, switch, button and status indicator. Optional inline pill via pillText. README documents the intentional View/Figma divergences.
DaxListItemContentDetector keeps the leading/trailing slots to their receiver-scope composables (resolved dynamically, so it needs no update as scope members change). DaxListItemColorUsageDetector requires primaryTextColor/secondaryTextColor to resolve to DuckDuckGoTheme semantic colours. Both registered in DuckDuckGoIssueRegistry, with tests.
3174581 to
d6208f2
Compare
d6208f2 to
753925d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
753925d to
73d85e3
Compare
73d85e3 to
f5f5a70
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f5f5a70. Configure here.
Reworks the one-line, two-line and settings list-item showcase screens to present each legacy View row beside its Compose equivalent (DaxOneLineListItem, DaxTwoLineListItem, DaxSettingsListItem), captioned "View" and "Compose". Rows the Compose API intentionally does not cover (16dp small and 40dp extra-large leading icons, rounded icon background) are flagged "View (no Compose equivalent)". Converts the three showcase layouts from ConstraintLayout to LinearLayout, drives the interleaved ComposeView slots from ComponentViewHolder, and removes the standalone component_compose_list_item.xml now that the demos live in the per-type layouts. Caption strings are added to a new design-system-internal donottranslate.xml. Also migrates the DialogsFragment call-sites to the new DaxOneLineListItem API (text -> primaryText, leadingIcon -> scoped leadingContent).
Scrolls to and asserts the three Compose list-item section headers and representative rows in the ADS components flow, in lockstep with the showcase strings.
f5f5a70 to
bc35e80
Compare
| return listOf( | ||
| SECTION_HEADER_LIST_ITEM, | ||
| SINGLE_LINE_LIST_ITEM, | ||
| TWO_LINE_LIST_ITEM, | ||
| SETTINGS_LIST_ITEM, | ||
| MENU_ITEM, | ||
| POPUP_MENU_ITEM, | ||
| ) |
There was a problem hiding this comment.
Why was this re-formatted?
landomen
left a comment
landomen
left a comment
There was a problem hiding this comment.
Left a few non-blocking comments otherwise it looks good
| modifier = modifier | ||
| .height(DaxPillDefaults.Height) | ||
| .clip(RoundedCornerShape(DaxPillDefaults.CornerRadius)) | ||
| .background(Yellow50) |
There was a problem hiding this comment.
This uses Yellow50 directly, but should we go through DuckDuckGoPillColors to match other components?
There was a problem hiding this comment.
Yes I agree, same for Text color.
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| Text( | ||
| text = text.uppercase(), |
There was a problem hiding this comment.
This could in theory be the callers responsibility, but considering existing DaxYellowPill already does this it's fine.
There was a problem hiding this comment.
All examples of Pill in the design system use capitalisation, so I'd like to enforce it and if something changes, we can update it 👍
| * Fixed palette in both light and dark (DuckDuckGo yellow background, black text) — it is not | ||
| * theme-tokened and draws its own text colour, matching the View [DaxYellowPill]. Callers supply | ||
| * their own (localised) label. | ||
| * |
There was a problem hiding this comment.
Missing Figma and Asana links
| * The primary label defaults to a single ellipsised line; the secondary caption is unbounded (wraps), | ||
| * matching the View. Sibling overloads accept [AnnotatedString] for HTML / inline-styled text. | ||
| * | ||
| * Figma reference: https://www.figma.com/design/BOHDESHODUXK7wSRNBOHdu/%F0%9F%A4%96-Android-Components?node-id=6032-13665 |
There was a problem hiding this comment.
Missing Asana link like other components have
| modifier: Modifier = Modifier, | ||
| primaryTextColor: Color = DuckDuckGoTheme.textColors.primary, | ||
| secondaryTextColor: Color = DuckDuckGoTheme.textColors.secondary, | ||
| pillText: String? = null, |
There was a problem hiding this comment.
Would it make sense to expose this as a slot where callers put DaxPill vs a String?
There was a problem hiding this comment.
Yeah this is a good point, does it make sense to make this slottable and do something similar to DaxListItemTrailingScope to enforce that for now we can only add a DaxPill?
| primaryText: String, | ||
| secondaryText: String, |
There was a problem hiding this comment.
Would it make sense to expose this as a slot where callers put DaxText vs a String? That also removes the need for primaryTextColor and secondaryTextColor.
I get the benefit of keeping it like now, it also matches existing XML. Moving to a more slot-based component makes it more Compose-like, but also adds more work for the caller and enables invalid usages (for which we'd need more lint rules).
I'm okay either way, just want to open a discussion since we went with more open slot approach for some other components.
There was a problem hiding this comment.
I don't believe there are any usages of ListItem's without text, therefore I think I prefer to lock it down and only allow Strings to enforce the API without the need of Lint. If at some point we need a ListItem that at it's core does not have text, then I think it would be appropriate to move to slots as you mention 👍
| * Settings row: one-line list item whose trailing element is a status indicator. | ||
| * | ||
| * The trailing slot is fixed to a [DaxListItemTrailingScope.StatusIndicator] driven by [status]; it is not a free slot like the other variants. | ||
| * |
There was a problem hiding this comment.
Missing Figma and Asana links
| var checked by remember { mutableStateOf(false) } | ||
| DaxTwoLineListItem( | ||
| primaryText = "Two Line Item Two Line Item Two Line Item Two Line Item", | ||
| secondaryText = "In disabled state", |
There was a problem hiding this comment.
Technically, this one is not in a disabled state since there's no enabled=false 🕵️
| import com.duckduckgo.common.ui.compose.theme.DuckDuckGoTheme | ||
|
|
||
| @Composable | ||
| internal fun DaxListItem( |
There was a problem hiding this comment.
We should add KDocs for this composable as well, even if it’s internal
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| if (leadingContent != null) { | ||
| leadingScope.leadingContent() |
There was a problem hiding this comment.
This is the way to enforce that the leading content is part of DaxListItemLeadingScope, right?
| } | ||
|
|
||
| @Stable | ||
| class DaxListItemLeadingScope internal constructor(private val parentEnabled: Boolean) { |
There was a problem hiding this comment.
Should we add KDocs here as well?
| } | ||
|
|
||
| @Stable | ||
| class DaxListItemTrailingScope internal constructor(private val parentEnabled: Boolean) { |
There was a problem hiding this comment.
Should we add KDocs?
| */ | ||
| @Composable | ||
| fun DaxTwoLineListItem( | ||
| primaryText: AnnotatedString, |
There was a problem hiding this comment.
Just making sure, we don’t need a variant with primary text as AnnotatedString and secondary text as String, right? From a design perspective I don’t think it makes sense but just want to make sure that we didn’t miss this component variant.
There was a problem hiding this comment.
Yeah are there any examples of this in the codebase where the current View version's primary text needs an Annotated string?
catalinradoiu
left a comment
catalinradoiu
left a comment
There was a problem hiding this comment.
Just some small comments, but overall it looks good to me.
| } | ||
| val leadingScope = DaxListItemLeadingScope(enabled) | ||
| val trailingScope = DaxListItemTrailingScope(enabled) | ||
| Row( |
There was a problem hiding this comment.
No space between Row and declared properties. It would be easier to read if there were. Is there anything we could apply to Spotless to enforce this?
| .alpha(if (effectiveEnabled) 1f else DaxListItemDefaults.DisabledAlpha), | ||
| ) | ||
| } else { | ||
| androidx.compose.material3.Icon( |
There was a problem hiding this comment.
Why do we have the full import here? It should just be Icon
| leadingScope.leadingContent() | ||
| Spacer(Modifier.width(DaxListItemDefaults.LeadingGap)) | ||
| } | ||
| Column(Modifier.weight(1f).alpha(if (enabled) 1f else DaxListItemDefaults.DisabledAlpha)) { |
There was a problem hiding this comment.
No space between Column and declared properties. It would be easier to read if there were. Is there anything we could apply to Spotless to enforce this?
| DaxPill(text = pillText) | ||
| } | ||
| } | ||
| if (secondaryText != null) { |
There was a problem hiding this comment.
No space between the if statement and previous Row code.
| ) | ||
| } | ||
| } | ||
| if (trailingContent != null) { |
There was a problem hiding this comment.
No space between the if statement and previous code.
| DaxListItemTrailingIconSize.Small -> DaxListItemDefaults.TrailingIconSmall | ||
| DaxListItemTrailingIconSize.Medium -> DaxListItemDefaults.TrailingIconMedium | ||
| } | ||
| if (onClick != null) { |
There was a problem hiding this comment.
No space between the if statement and previous code.
There was a problem hiding this comment.
I went over the code and aside from the existing comments I didn't find any issues. Nice work! 🏅
When I was looking at the View vs. Compose differences, I noticed some UI and behavior differences, like:
- Different leading icon sizes and color (globe) (smaller in Compose)
- Different ripple size when tapping the menu icon (smaller in Compose)
- No leading icon click handler in Compose
- Smaller padding between primary and secondary text in Compose
I was looking for the documentation for the differences but I couldn't find any README and this is empty. Is it somewhere else?
4 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1215496415658080/task/1211659112661214?focus=true
Tech Design URL (if applicable):
Description
Adds the Compose list item family to the design system, following on from the buttons and text inputs.
DaxOneLineListItem,DaxTwoLineListItemandDaxSettingsListItem. All are thin variants over an internalDaxListItemcore with receiver-scoped leading/trailing slots, so callers can only place content the design system allowspillTextDaxPill, a standalone status pill matching the ViewDaxYellowPill. The caller supplies the (localised) label so the design system owns no product copyprimaryTextColor/secondaryTextColorto resolve toDuckDuckGoThemesemantic coloursPreviewSurfacefor full-width components that carry their own internal paddingThe intentional divergences from the View components and Figma are documented in the module README.
Steps to test this PR
List items showcase
Lint rules
./gradlew :lint-rules:testand check the new detector tests passMaestro
maestro test .maestro/ads_preview_flows/1-_design-system-components.yamlwith the app installedUI changes
Note
Medium Risk
New public design-system APIs and a breaking reshape of
DaxOneLineListItem; production impact is small (one dialog call site updated) but adopters must migrate parameters and respect new lint rules.Overview
Introduces Compose list items (
DaxOneLineListItem,DaxTwoLineListItem,DaxSettingsListItem) on a shared internalDaxListItem, with receiver-scopedleadingContent/trailingContent(icons, switch, button, status indicator) and optionalpillTextvia newDaxPill.DaxOneLineListItemis reworked from the prior Material3ListItemstub to the design-system API (primaryText, slots, pills, annotated text overloads).DialogsFragmentis updated to that API.The internal ADS showcase interleaves each View row with a matching Compose
ComposeView, adds View / Compose / no-equivalent captions, passesisDarkThemeinto list-item view holders, and refactors list layouts from ConstraintLayout to LinearLayout.PreviewSurfaceis added for full-width previews.Lint registers
DaxListItemContentDetectorandDaxListItemColorUsageDetector(with tests). Maestro scrolls/asserts Single Line, Two Line, and Settings showcase sections.Reviewed by Cursor Bugbot for commit bc35e80. Bugbot is set up for automated code reviews on this repo. Configure here.