refactor: align List tokens, spacing and typography with MD3 - #5041
refactor: align List tokens, spacing and typography with MD3#5041adam-sajko wants to merge 6 commits into
Conversation
MikitasK
left a comment
MikitasK
left a comment
There was a problem hiding this comment.
good job 👏 overall, this refactoring looks solid 👍
just a few thing to consider before merge:
8e2f57e to
b81344f
Compare
| <MaterialCommunityIcon | ||
| name={isExpanded ? 'chevron-up' : 'chevron-down'} | ||
| color={descriptionColor} | ||
| color={theme.colors[ListTokens.trailingIconColor]} |
There was a problem hiding this comment.
Should the chevron use the expand set rather than the generic list-item one? On the
Lists specs md.comp.list.expand.collapsed.list-item.trailing-icon.icon.color and its expanded twin both resolve to md.sys.color.on-surface, where md.comp.list.list-item.trailing-icon.color is on-surface-variant.
| setIsDescriptionMultiline(nativeEvent.lines.length >= 2); | ||
| }; | ||
|
|
||
| const getVerticalPaddingStyle = () => { |
There was a problem hiding this comment.
react-native-web doesn't implement onTextLayout, and a function description never gets it either, so in both cases a wrapped description stays on the 14dp tier and renders 92dp against md.comp.list.list-item.three-line.container.height = 88dp. Would minHeight: description ? 72 : 56 with a flat 12dp padding work instead? It lands on 56 / 72 / 88 without measuring anything.
There was a problem hiding this comment.
I did what you suggested with one exception. I kept onTextLayout. The height no longer reads it, but the same flag still top-aligns the left and right elements on three line items, which was all it did before this PR. Please take another look :)
6a7b135 to
0dfd5f9
Compare
0dfd5f9 to
d92fdfa
Compare
| const { | ||
| wrapper: { queryByText, getByText, findByText }, | ||
| } = await setup({ enterTouchDelay: 50, leaveTouchDelay: 0 }); | ||
| } = await setup({ enterTouchDelay: 50, leaveTouchDelay: 100 }); |
| import type { ColorRole } from '../../theme/types'; | ||
|
|
||
| const sizes = { | ||
| verticalPadding: 12, |
There was a problem hiding this comment.
could we avoid applying 12dp root padding to every row?
with the PR’s own 40dp avatar example, one-line item becomes 64dp (40 + 12 + 12) instead of 56dp, List.Image (56dp) becomes 80dp instead of 72dp. compose uses 8dp vertical padding for one/two-line items & 12dp only for 3-line items
maybe we can preserve measurement-free 56/72/88dp text rows by using 8dp container padding plus 4dp content padding with the same 4dp offset for top-aligned accessories? and could we also cover a 40dp avatar or List.Image case?
5 participants
Motivation
List.Item and List.Accordion now follow the MD3 list spec for padding, type, and colour instead of hardcoded sizes and the old expanded primary tint.
Visual changes
alignSelfinstead of the old fixed 40dp box, so it shifts a little on two-line accordions.Two cleanups in
utils.ts.getAccordionColorsonly existed to return the primary tint on expand, so it goes away together with its tests.getLeftStylesandgetRightStyleswere the same function twice, now one helper with the same output.Related issue
Related to #4976
Test plan