-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
refactor: align List tokens, spacing and typography with MD3 #5041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,8 +13,9 @@ import type { | |
| } from 'react-native'; | ||
|
|
||
| import { ListAccordionGroupContext } from './ListAccordionGroup'; | ||
| import { ListTokens } from './tokens'; | ||
| import type { ListChildProps, Style } from './utils'; | ||
| import { getAccordionColors, getLeftStyles } from './utils'; | ||
| import { getLeftStyles } from './utils'; | ||
| import { useLocale } from '../../core/locale'; | ||
| import { useInternalTheme } from '../../core/theming'; | ||
| import type { ThemeProp } from '../../types'; | ||
|
|
@@ -232,20 +233,24 @@ const ListAccordion = ({ | |
| ? groupContext.expandedId === id | ||
| : expandedInternal; | ||
|
|
||
| const { descriptionColor, titleTextColor } = getAccordionColors({ | ||
| theme, | ||
| isExpanded, | ||
| }); | ||
| const titleTextColor = theme.colors[ListTokens.headlineColor]; | ||
| const descriptionColor = theme.colors[ListTokens.supportingTextColor]; | ||
|
|
||
| const handlePress = | ||
| groupContext && id !== undefined | ||
| ? () => groupContext.onAccordionPress(id) | ||
| : handlePressAction; | ||
| return ( | ||
| <View> | ||
| <View style={{ backgroundColor: theme?.colors?.background }}> | ||
| <View | ||
| style={{ backgroundColor: theme.colors[ListTokens.containerColor] }} | ||
| > | ||
| <TouchableRipple | ||
| style={[styles.container, style]} | ||
| style={[ | ||
| styles.container, | ||
| description ? styles.containerTwoLine : styles.containerOneLine, | ||
| style, | ||
| ]} | ||
| onPress={handlePress} | ||
| onLongPress={onLongPress} | ||
| delayLongPress={delayLongPress} | ||
|
|
@@ -264,16 +269,16 @@ const ListAccordion = ({ | |
| > | ||
| {left | ||
| ? left({ | ||
| color: isExpanded ? theme.colors?.primary : descriptionColor, | ||
| color: theme.colors[ListTokens.leadingIconColor], | ||
| style: getLeftStyles(alignToTop, description), | ||
| }) | ||
| : null} | ||
| <View style={[styles.contentItem, styles.content, contentStyle]}> | ||
| <Text | ||
| variant="bodyLarge" | ||
| selectable={false} | ||
| numberOfLines={titleNumberOfLines} | ||
| style={[ | ||
| styles.title, | ||
| { | ||
| color: titleTextColor, | ||
| }, | ||
|
|
@@ -285,10 +290,10 @@ const ListAccordion = ({ | |
| </Text> | ||
| {description ? ( | ||
| <Text | ||
| variant="bodyMedium" | ||
| selectable={false} | ||
| numberOfLines={descriptionNumberOfLines} | ||
| style={[ | ||
| styles.description, | ||
| { | ||
| color: descriptionColor, | ||
| }, | ||
|
|
@@ -301,20 +306,15 @@ const ListAccordion = ({ | |
| </Text> | ||
|
Comment on lines
277
to
306
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same suggestion here |
||
| ) : null} | ||
| </View> | ||
| <View | ||
| style={[ | ||
| styles.trailingItem, | ||
| description ? styles.multiline : undefined, | ||
| ]} | ||
| > | ||
| <View style={styles.trailingItem}> | ||
| {right ? ( | ||
| right({ | ||
| isExpanded: isExpanded, | ||
| }) | ||
| ) : ( | ||
| <MaterialCommunityIcon | ||
| name={isExpanded ? 'chevron-up' : 'chevron-down'} | ||
| color={descriptionColor} | ||
| color={theme.colors[ListTokens.trailingIconColor]} | ||
| size={24} | ||
| direction={direction} | ||
| /> | ||
|
|
@@ -349,29 +349,22 @@ ListAccordion.displayName = 'List.Accordion'; | |
|
|
||
| const styles = StyleSheet.create({ | ||
| container: { | ||
| paddingVertical: 8, | ||
| paddingRight: 24, | ||
| paddingRight: ListTokens.trailingSpace, | ||
| }, | ||
| row: { | ||
| flexDirection: 'row', | ||
| marginVertical: 6, | ||
| }, | ||
| multiline: { | ||
| height: 40, | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| containerOneLine: { | ||
| paddingVertical: ListTokens.oneLineVerticalPadding, | ||
| }, | ||
| title: { | ||
| fontSize: 16, | ||
| containerTwoLine: { | ||
| paddingVertical: ListTokens.twoLineVerticalPadding, | ||
| }, | ||
| description: { | ||
| fontSize: 14, | ||
| row: { | ||
| flexDirection: 'row', | ||
| }, | ||
| contentItem: { | ||
| paddingLeft: 16, | ||
| paddingLeft: ListTokens.leadingSpace, | ||
| }, | ||
| trailingItem: { | ||
| marginVertical: 6, | ||
| alignSelf: 'center', | ||
| paddingLeft: 8, | ||
| }, | ||
| child: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,6 +10,7 @@ import type { | |||||||||||||
| ViewStyle, | ||||||||||||||
| } from 'react-native'; | ||||||||||||||
|
|
||||||||||||||
| import { ListTokens } from './tokens'; | ||||||||||||||
| import { getLeftStyles, getRightStyles } from './utils'; | ||||||||||||||
| import type { Style } from './utils'; | ||||||||||||||
| import { useInternalTheme } from '../../core/theming'; | ||||||||||||||
|
|
@@ -179,18 +180,15 @@ const ListItem = ({ | |||||||||||||
| selectable: false, | ||||||||||||||
| ellipsizeMode: descriptionEllipsizeMode, | ||||||||||||||
| color: descriptionColor, | ||||||||||||||
| fontSize: styles.description.fontSize, | ||||||||||||||
| fontSize: theme.fonts.bodyMedium.fontSize, | ||||||||||||||
| }) | ||||||||||||||
| ) : ( | ||||||||||||||
| <Text | ||||||||||||||
| variant="bodyMedium" | ||||||||||||||
| selectable={false} | ||||||||||||||
| numberOfLines={descriptionNumberOfLines} | ||||||||||||||
| ellipsizeMode={descriptionEllipsizeMode} | ||||||||||||||
| style={[ | ||||||||||||||
| styles.description, | ||||||||||||||
| { color: descriptionColor }, | ||||||||||||||
| descriptionStyle, | ||||||||||||||
| ]} | ||||||||||||||
| style={[{ color: descriptionColor }, descriptionStyle]} | ||||||||||||||
| onTextLayout={onDescriptionTextLayout} | ||||||||||||||
| maxFontSizeMultiplier={descriptionMaxFontSizeMultiplier} | ||||||||||||||
| > | ||||||||||||||
|
Comment on lines
186
to
194
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about passing resolved in current implementation component-level font overrides are used for render-prop titles & descriptions, but plain text still reads typography from the surrounding provider |
||||||||||||||
|
|
@@ -200,35 +198,40 @@ const ListItem = ({ | |||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const renderTitle = () => { | ||||||||||||||
| const titleColor = theme.colors.onSurface; | ||||||||||||||
| const titleColor = theme.colors[ListTokens.headlineColor]; | ||||||||||||||
|
|
||||||||||||||
| return typeof title === 'function' ? ( | ||||||||||||||
| title({ | ||||||||||||||
| selectable: false, | ||||||||||||||
| ellipsizeMode: titleEllipsizeMode, | ||||||||||||||
| color: titleColor, | ||||||||||||||
| fontSize: styles.title.fontSize, | ||||||||||||||
| fontSize: theme.fonts.bodyLarge.fontSize, | ||||||||||||||
| }) | ||||||||||||||
| ) : ( | ||||||||||||||
| <Text | ||||||||||||||
| variant="bodyLarge" | ||||||||||||||
| selectable={false} | ||||||||||||||
| ellipsizeMode={titleEllipsizeMode} | ||||||||||||||
| numberOfLines={titleNumberOfLines} | ||||||||||||||
| style={[styles.title, { color: titleColor }, titleStyle]} | ||||||||||||||
| style={[{ color: titleColor }, titleStyle]} | ||||||||||||||
| maxFontSizeMultiplier={titleMaxFontSizeMultiplier} | ||||||||||||||
| > | ||||||||||||||
| {title} | ||||||||||||||
| </Text> | ||||||||||||||
|
Comment on lines
211
to
220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same suggestion here |
||||||||||||||
| ); | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const descriptionColor = theme.colors.onSurfaceVariant; | ||||||||||||||
| const descriptionColor = theme.colors[ListTokens.supportingTextColor]; | ||||||||||||||
|
|
||||||||||||||
| return ( | ||||||||||||||
| <TouchableRipple | ||||||||||||||
| {...rest} | ||||||||||||||
| ref={ref} | ||||||||||||||
| style={[styles.container, style]} | ||||||||||||||
| style={[ | ||||||||||||||
| styles.container, | ||||||||||||||
| description ? styles.containerTwoLine : styles.containerOneLine, | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about descriptions with 2+ lines? they currently keep
Suggested change
|
||||||||||||||
| style, | ||||||||||||||
| ]} | ||||||||||||||
| onPress={onPress} | ||||||||||||||
| theme={theme} | ||||||||||||||
| testID={testID} | ||||||||||||||
|
|
@@ -265,22 +268,20 @@ ListItem.displayName = 'List.Item'; | |||||||||||||
|
|
||||||||||||||
| const styles = StyleSheet.create({ | ||||||||||||||
| container: { | ||||||||||||||
| paddingVertical: 8, | ||||||||||||||
| paddingRight: 24, | ||||||||||||||
| paddingRight: ListTokens.trailingSpace, | ||||||||||||||
| }, | ||||||||||||||
| containerOneLine: { | ||||||||||||||
| paddingVertical: ListTokens.oneLineVerticalPadding, | ||||||||||||||
| }, | ||||||||||||||
| containerTwoLine: { | ||||||||||||||
| paddingVertical: ListTokens.twoLineVerticalPadding, | ||||||||||||||
| }, | ||||||||||||||
| row: { | ||||||||||||||
| width: '100%', | ||||||||||||||
| flexDirection: 'row', | ||||||||||||||
| marginVertical: 6, | ||||||||||||||
| }, | ||||||||||||||
| title: { | ||||||||||||||
| fontSize: 16, | ||||||||||||||
| }, | ||||||||||||||
| description: { | ||||||||||||||
| fontSize: 14, | ||||||||||||||
| }, | ||||||||||||||
| item: { | ||||||||||||||
| paddingLeft: 16, | ||||||||||||||
| paddingLeft: ListTokens.leadingSpace, | ||||||||||||||
| }, | ||||||||||||||
| content: { | ||||||||||||||
| flexShrink: 1, | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import type { ColorRole } from '../../theme/types'; | ||
|
|
||
| const sizes = { | ||
| oneLineVerticalPadding: 16, | ||
| twoLineVerticalPadding: 14, | ||
| leadingSpace: 16, | ||
| trailingSpace: 24, | ||
| } as const; | ||
|
|
||
| const colors = { | ||
| containerColor: 'surface', | ||
| headlineColor: 'onSurface', | ||
| supportingTextColor: 'onSurfaceVariant', | ||
| leadingIconColor: 'onSurfaceVariant', | ||
| trailingIconColor: 'onSurfaceVariant', | ||
| } as const satisfies Record<string, ColorRole>; | ||
|
|
||
| export const ListTokens = { ...sizes, ...colors }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same suggestion here