refactor: align List tokens, spacing and typography with MD3 - #5041
refactor: align List tokens, spacing and typography with MD3#5041adam-sajko wants to merge 8 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?
There was a problem hiding this comment.
Done. However, I haven't added the 4dp offset for accessories because it would be tricky, and the MD3 spec images in Figma directly state 8dp or 12dp vertical padding for the entire container
JKobrynski
left a comment
JKobrynski
left a comment
There was a problem hiding this comment.
Good job, LGTM! @satya164 want to take a look?
|
Note: I didn't modify the checkbox element itself, but it renders differently than specified in the MD3 specs. Our checkbox is normally rendered within a 40x40dp container. However, for three-line text that is top-aligned, the 40x40dp container is dismissed, and the checkbox scales down to 24dp. I'd like to do it separately. Reference: |
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.
ACs
https://m3.material.io/components/lists/specs
Related issue
Related to #4976
Test plan