List items are misbehaving

[drewvolz]

I think something is mutating? When I scroll the menu list, it sort of... jitters. After that strange UI animation, it inserts a whole bunch of duplicate items in the list each time it loads. It continues to do so when I continue to scroll. Could you help debug this, please, @hannesmcman?

[drewvolz]

Another thing I'm noticing that we might turn into a separate issue:

Pull to Refresh when filters are applied -- the button does not change colors nor are the filters listed on the popup, nor are they applied to the list after PTR-ing. It takes tapping the button again for the filters to be applied.

Is that FAD / desired behavior?

[hannesmcman]

@drewvolz Can I see a gif of the bug? What do you mean by the menu list?

[drewvolz]

What do you mean by the menu list?

The menu list is the list of menu items (sorry!)

[hawkrives]

@drewvolz your issue in the first comment (not the OP, afaik) is cause by the removal of deriveStateFromProps. Without that, we never update the internal state of the filter popup's container.

Another approach, and the way I was kinda thinking about exploring, would be to add a "key" to the FilterToolbarButton based around the values in the filter, such that changing a filter from outside would unmount and remount the component.

If you look in React's docs for controlled/uncontrolled inputs, this is one of their suggestions.

[drewvolz]

Another approach, and the way I was kinda thinking about exploring, would be to add a "key" to the FilterToolbarButton based around the values in the filter, such that changing a filter from outside would unmount and remount the component.

+1

[hawkrives]

The problem is that a good key is a hash of the object identity in this case, which is not something JS exposes.

And that's where I left off.

I started going down the rabbit hole of a UUID that changes when the object is changed, and abandoned it there in my local branch, but I think there's potential in that direction still. Maybe.

[hannesmcman]

#3350 addresses the issue mentioned in the first comment, but not the OP

[hawkrives]

@drewvolz could we get a screen recording of the OP?

[drewvolz]

Sure. Here is what I am seeing.

[hannesmcman]

@drewvolz Can we rename this issue to reflect the OP? I don't think it has anything to do with filters..

[drewvolz]

FWIW I'm seeing this same issue on Course Search.

[drewvolz]

Please rename it to what seems right. I've renamed it to "List items are misbehaving"

[hannesmcman]

@drewvolz LGTM. I am going to create a separate issue for clearing of filters with PTR.

[hannesmcman]

Updated gif with debug={true}:

[hawkrives]

Can we git bisect to find the offending merge? (Can bisect be configured to bisect only merge commits?)

[rye]

@hawkrives without specifying all merge commits that you want to test, I don't think so.

[hannesmcman]

I'm not entirely sure this isn't a bug with SectionList's rendering itself...look how in debug mode the weird behavior happens when the render window is resizing itself..

[hawkrives]

It may be, but it seems really odd that this suddenly showed up in the past few days.

[drewvolz]

I've played a little with the props that <SectionList /> takes.

Passing stickySectionHeadersEnabled={false} into the <SectionList /> seems to prevent the "jumping" and insertion of duplicate rows. I think this might be an overall upgrade issue.

facebook/react-native#20956
facebook/react-native#21468
facebook/react-native#22444
facebook/react-native#20127

AAO-React-Native/modules/food-menu/fancy-menu.js

Lines 199 to 212 in 69e1f38

	<SectionList
	ItemSeparatorComponent={Separator}
	ListEmptyComponent={messageView}
	ListHeaderComponent={header}
	data={filters}
	keyExtractor={this.keyExtractor}
	onRefresh={this.props.onRefresh}
	refreshing={this.props.refreshing}
	renderItem={this.renderItem}
	renderSectionHeader={this.renderSectionHeader}
	sections={(groupedMenuData: any)}
	style={styles.inner}
	windowSize={5}
	/>

[drewvolz]

Would anyone be opposed to adding stickySectionHeadersEnabled={false} to our <SectionList /> components?

[hawkrives]

Yes

[hannesmcman]

If we can find a fix that doesn't involve disabling sticky section headers I would prefer that as well..

[rye]

If we are to meet our release goal of January 1, I don't think there's any other way to fix this, since what we're seeing seems to be an internal issue with RN. (See upstream issues that Drew linked.)

N.b. a fix PR is up at facebook/react-native#22025 but given that PR has seen no review or anything from the upstreamers for the last 30 days, (classic) I'm willing to bet that it's probably not going to get reviewed until after our deadline.

I think it's best to apply that prop with a note to self to fix it, as seems to be the approach in #3353.

[drewvolz]

What’re the risks of testing that patch ourselves and applying it to our repo? Do we feel like going with disabling sticky headers, testing the upstream patch, or something else?

[drewvolz]

I have two PRs out now:

• Apply upstream patch from #2205 #3357 which applies the upstream patch @rye found within RN #22025
• Disable sticky headers for menus and courses #3353 which disables sticky headers for Menus and Course Search until further notice

I'm in favor of #3357 instead of #3353. After running it myself, it appears to fix what we were seeing.

[drewvolz]

Closed by #3357