Repository navigation
Conversation
…s-spacing # Conflicts: # src/components/List/ListItem.tsx # src/components/__tests__/ListAccordion.test.tsx # src/components/__tests__/ListImage.test.tsx
…nd-accordion-animation
…ist-typed-slots # Conflicts: # src/components/List/ListItem.tsx
…nd-accordion-animation
…nd-accordion-animation
…s-spacing # Conflicts: # src/components/List/ListAccordion.tsx # src/components/List/ListItem.tsx # src/components/__tests__/ListAccordion.test.tsx
…nd-accordion-animation # Conflicts: # src/components/List/ListAccordion.tsx # src/components/List/ListItem.tsx # src/components/__tests__/__snapshots__/ListAccordion.test.tsx.snap # src/components/__tests__/__snapshots__/ListItem.test.tsx.snap # src/components/__tests__/__snapshots__/ListSection.test.tsx.snap
…ist-typed-slots # Conflicts: # src/components/List/ListItem.tsx
|
Found potential problems with the pull request:
|
|
Review by Claude iOS and AndroidList.Accordion: rows below the accordion jump on expand and collapse Steps: open the List.Accordion example and tap "Expandable list item" to expand it, then tap it again to collapse it. What happens: the accordion content animates its own height and opacity, but the rows below it do not follow. On expand, "Start expanded" jumps to its final position at once and the content fades into an empty gap. On collapse, the rows below snap up at once and the fading "List item 2" draws on top of the next accordion's "List item 1". What should happen: the rows below move together with the content height, with no gap and no overlap. (via Claude, verify if it makes sense) WebList.Accordion: no expand animation and a Reanimated warning Steps: open the List.Accordion example on web and expand any accordion. What happens: the content appears and disappears with no animation, and the console logs What should happen: the same animation as on native, with no warning. (via Claude, verify if it makes sense) List.Item: three-line rows keep the two-line padding and centered accessories Steps: open the List.Item example on web in a narrow window and look at the rows whose supporting text wraps to two lines. What happens: the rows are 80px tall with 8px vertical padding, and the leading icon stays vertically centered. What should happen: 88px rows with 12px vertical padding and the leading icon aligned to the top, as on iOS and Android. (via Claude, verify if it makes sense) |
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
See the inline comments. (via Claude, verify if it makes sense)
| const expandAnimation = React.useCallback( | ||
| (values: EntryAnimationsValues) => { | ||
| 'worklet'; | ||
| return { | ||
| initialValues: { height: 0, opacity: 0 }, | ||
| animations: { | ||
| height: withTiming(values.targetHeight, timingConfig), | ||
| opacity: withTiming(1, timingConfig), | ||
| }, | ||
| }; | ||
| }, | ||
| [timingConfig] | ||
| ); | ||
|
|
||
| const collapseAnimation = React.useCallback( | ||
| (values: ExitAnimationsValues) => { | ||
| 'worklet'; | ||
| return { | ||
| initialValues: { height: values.currentHeight, opacity: 1 }, | ||
| animations: { | ||
| height: withTiming(0, timingConfig), | ||
| opacity: withTiming(0, timingConfig), | ||
| }, | ||
| }; | ||
| }, | ||
| [timingConfig] | ||
| ); |
There was a problem hiding this comment.
The entering and exiting layout animations change the height of the content view only. The rows below the accordion do not follow it. On expand they jump to their final position and the content fades into an empty gap. On collapse they snap up at once and the fading content draws over them. This happens on iOS and Android:
On web, custom entering and exiting functions are not supported, so there is no animation and Reanimated logs Couldn't load entering/exiting animation on every expand.
The height needs to change in layout so that the siblings move with it, and it needs to work on web. For example, keep the content mounted in a clipped wrapper and transition its height from the measured content height, or drop the height part and keep only the opacity. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
I couldn’t reproduce the original jump on expand or collapse, even with medium2 raised to 3000ms on native. The rows below stay in place.But I did find a different issue: when the last item inside an accordion collapses, the content below the accordion overlaps the collapsing item.
| const chevronProgress = useSharedValue(isExpanded ? 1 : 0); | ||
|
|
||
| React.useEffect(() => { | ||
| chevronProgress.value = withTiming(isExpanded ? 1 : 0, timingConfig); | ||
| }, [isExpanded, chevronProgress, timingConfig]); | ||
|
|
||
| const chevronStyle = useAnimatedStyle(() => ({ | ||
| transform: [{ rotate: `${chevronProgress.value * 180}deg` }], | ||
| })); |
There was a problem hiding this comment.
The chevron rotation is a transition between two states (isExpanded false and true). The project uses Reanimated CSS transitions for that case, as in src/components/Checkbox/Checkbox.tsx: put transform: [{ rotate: isExpanded ? '180deg' : '0deg' }] on the Animated.View with transitionProperty, transitionDuration (0 when reduceMotion is on) and transitionTimingFunction. That removes the shared value, the effect and useAnimatedStyle. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
It’s possible to switch the chevron to the Checkbox-style CSS transition.
| @@ -169,15 +186,23 @@ const ListItem = ({ | |||
| const theme = useInternalTheme(themeOverrides); | |||
| const { hasLeft } = React.useContext(ListAccordionContext); | |||
| const shouldIndent = hasLeft && !left && !right; | |||
There was a problem hiding this comment.
shouldIndent ignores the new slots. Inside a List.Accordion that has left, an item with leading gets the extra 40px indent on top of its leading element, while the same item with left does not. An item with only trailing is indented, while one with only right is not.
| const shouldIndent = hasLeft && !left && !right; | |
| const shouldIndent = hasLeft && !left && !right && !leading && !trailing; |
(via Claude, verify if it makes sense)
| style={[ | ||
| styles.container, | ||
| description ? styles.containerTwoLine : styles.containerOneLine, | ||
| isDescriptionMultiline && styles.containerThreeLine, |
There was a problem hiding this comment.
The three-line padding depends on onTextLayout, which react-native-web does not fire. On web a row whose supporting text wraps stays at 80px with 8px padding and a centered leading element, instead of 88px with 12px padding and a top-aligned leading element:
The same applies to List.Accordion. The line count needs a source that works on web too, for example the height from onLayout of the description compared with the bodyMedium line height. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
Status: addressed by a separate PR, callstack/react-native-paper#5091, which is open.
callstack/react-native-paper#5091 replaces onTextLayout with a measurement that works on web. It’s a PoC and stacked on this branch, so it can only merge after this PR does.
| style, | ||
| ]} | ||
| onPress={onPress} | ||
| aria-selected={selected} |
There was a problem hiding this comment.
On web this renders aria-selected on a div with no role. ARIA only supports aria-selected on option, tab, row and gridcell, so screen readers do not announce the selected state there. The same applies to List.Accordion, which has role="button". iOS and Android do expose it (selected="true" in the Android accessibility tree). The web output needs a role that supports the state, or a state that the current role supports. A test for the exposed state is also missing. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
The current setup is consistent with other components: Chip and DrawerItem also pair role="button" with aria-selected. Changing only this PR would make it inconsistent.
Options, depending on the intended behavior:
Single-select list: role="option" inside a listbox, with aria-selected.
Highlighted current item: aria-current.
Accordion: keep role="button" and aria-expanded, and drop aria-selected.
Next step: decide the semantics, then apply them here and possibly file a follow-up for Chip and DrawerItem.
| /** | ||
| * Element to display in the leading slot. Takes precedence over `left`. | ||
| */ | ||
| leading?: React.ReactNode; | ||
| /** | ||
| * Element to display in the trailing slot. Takes precedence over `right`. | ||
| */ | ||
| trailing?: React.ReactNode; |
There was a problem hiding this comment.
leading and trailing add a second way to fill the same slots as left and right, with a silent precedence rule when both are passed. List.Accordion keeps only left and right, so the two components now have different APIs for the same row. Since the project does not keep backward compatibility by default, one API for both components is simpler: either replace left and right with the slots in both, or leave the slots out. (via Claude, verify if it makes sense)
There was a problem hiding this comment.
Before we settle the left/right question, we need a maintainer decision on how to handle the existing API. Which approach should be taken?
- Keep both and document precedence. left/right stay, and leading/trailing take priority when both are passed.
- Deprecate left/right in v6 and remove them later. Both sets work for now, with a deprecation warning on the old props.
- Remove left/right now. This is a BREAKING CHANGE, with a migration guide.
| it.each([ | ||
| ['a 40dp accessory', styles.avatar, undefined, 56], | ||
| ['a 56dp accessory', styles.image, 'Item description', 72], | ||
| ['a 64dp accessory', styles.video, undefined, 56], |
There was a problem hiding this comment.
This case passes without checking what the title says. A 64px leading element in a one-line row makes the row 80px tall, but the assertion only checks the minHeight: 56 style, which is the same with or without the accessory. The test also uses a plain View instead of List.Image, so the new marginVertical logic in ListImage is not covered. The tests in this file assert exact internal style values (minHeight, paddingVertical, alignSelf). The project test rules ask for public behavior instead, for example the rendered layout of List.Image in each row type. (via Claude, verify if it makes sense)











Motivation
Brings
List.ItemandList.Accordionin line with the latest MD3 list spec:leadingandtrailingslots onList.Item, which take precedence overleftandrightThis combines #5041, #5057 and #5042, which were stacked on each other.
Related issue
Related to #4976, #4930
Screenshots / Videos
List.Accordion


List.Item screenshots are in the comment below.
Test plan
Open the List example and:
leadingandtrailingslots render and pick up the item color, including in a selected row.