Skip to content
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

Fix issue with element below the anchor blinking during data loading. #53

Open
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

mbilenko-florio
Copy link
Contributor

@mbilenko-florio mbilenko-florio commented Jan 5, 2025

Anchors all elements below anchor to the bottom edge, which reduces visual glitches

@umardev500
Copy link

bump

@michbil
Copy link

michbil commented Jan 18, 2025

@jmeistrich I think I was able to crack this problem by allowing containers above anchor to grow up. This requires no additional opacity cycles and seems to be quite performant.
Downside is addition of additional view in the hierarchy, could this cause performance problem during recycling?

Now trying to cleanup code and fix numColumns regression, but overall i have good feeling about this solution.

@jmeistrich
Copy link
Contributor

Oh that's awesome!

Yes, I think that might be bad for performance since the top and bottom versions have different view hierarchy, so it wouldn't recycle well when switching between them. Is it possible to just change the style to use bottom instead of top rather than returning different hierarchy?

@michbil
Copy link

michbil commented Jan 18, 2025

@jmeistrich I can try to have two hierarchies, shallow hierarchy will be used when maintainVisibleContentPosition={false} and deeper for maintainVisibleContentPosition={true}.
If user don't need maintainVisibleContentPosition we will use minimal amount of views.

PS: we have orphaned LeanView component, does it still make sense to use it?

@jmeistrich
Copy link
Contributor

Ok that seems reasonable. So the bottom behavior requires the extra level of hierarchy? It can't just set bottom instead of top on the style?

Oh yeah, LeanView supposedly has slightly better perf but I lost track of it with all the experimenting. So we could use that whenever we need a simple View.

@michbil
Copy link

michbil commented Jan 18, 2025

@jmeistrich Just bottom style will require distance from the bottom of the scrollview's content to the top, and calculating it is super hard(lot of Animated magic is needed), i tried to make it few weeks ago, and it's pain in the ass.
Extra View doesn't look like big overhead to me, at least at this point. If you have another idea how to allow it grow up without extra view(maybe with transform?), I'm open to suggestions.

@jmeistrich
Copy link
Contributor

Oh right of course 😅. I think this looks like a good solution, an extra view isn't a big problem.

@mbilenko-florio mbilenko-florio marked this pull request as ready for review January 18, 2025 11:57
@jmeistrich
Copy link
Contributor

Just checking - are you still working on this or is it ready for merge? It's a good fix so I'd like to get it in if it's ready :)

@@ -1186,6 +1209,7 @@ const LegendListInner: <T>(props: LegendListProps<T> & { ref?: ForwardedRef<Lege
ListEmptyComponent={data.length === 0 ? ListEmptyComponent : undefined}
maintainVisibleContentPosition={maintainVisibleContentPosition}
scrollEventThrottle={scrollEventThrottle ?? (Platform.OS === "web" ? 16 : undefined)}
waitForInitialLayout={true}
Copy link

@michbil michbil Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmeistrich I think that waitForInitialLayout should be default option, and should be opted out when needed. Because that's behaviour most users are expecting. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's probably right. My general criteria is to default to expected and opt-in to optimized performance if it would alter expected behavior. I think you're probably right that would be expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this would always set it to true - should initialize the value to default to true at the top of the component?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right, i need to set default prop. Let me fix that!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmeistrich fixed

@michbil
Copy link

michbil commented Jan 23, 2025

@jmeistrich I have found another blinking bug, where old elements are blinking(because of momentarily invalid scroll), it looks i will need to bring back scroll position filter.
But that's not related to current bugfix, i will fix it in the separate PR. I've cleaned up this PR, and it's ready to be merged. Pls take a look at one comment I left you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants