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

bug: passing scrollRef causes element being pinned to top without respecting height of nodes before the scrollview #75

Open
hirbod opened this issue Jul 27, 2024 · 14 comments

Comments

@hirbod
Copy link
Contributor

hirbod commented Jul 27, 2024

Assume the following structure:

<TrueSheet>
  <MyHeaderComponent />
  <FlashList />
</TrueSheet>

All good so far. But once I pass the scrollRef (with FlashList, unlike FlatList, you need to pass the scrollableNode like so):

ref={(ref) => {
  if (scrollRef) {
    ;(scrollRef.current as unknown) = ref?.getScrollableNode()
  }
}}

It works fine. The main problem I have is that (at least on iOS; I haven't tested Android yet), there is a utility function called pinTo, which now pins the whole view behind MyHeaderComponent, thus not taking the element's height into account.

I've experimented with TrueSheetView and tested adding offsets like so:

rctScrollView.pinTo(view: contentView) { constraints in
    constraints.top?.constant = 50 // 50 is the height of my header
}

So this works. I think there are multiple ways to solve this problem:

  1. Add a HeaderComponent, similar to FooterComponent, and handle all of this under the hood.
  2. Ensure to run through all the siblings of TrueSheet before the actual ScrollView and automatically calculate the offset.
  3. Add a prop (like scrollViewOffset), and we pass a value to it, like 50
Before Patch After Patch
After Patch Before Patch

You may ask why I don't use HeaderComponent by FlashList? Well, because it does not keep the header fixed.

My example above is just a quick "test", not a fix. The offset needs to be reflected in all calculations etc, similar to the footer.

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

I thought of the same on the first version. But I ultimately decided not to.

The simplest solution to that problem is to add paddingTop to your FlashList.

I wanted to reduce the complexity of the constraints code. Had no choice with the FooterComponent since it needs to stick at the bottom.

FWIW I believe the "standard" is to use padding on the list and float the header, it's much consistent/performant when you want to animate it together with the list.

@hirbod
Copy link
Contributor Author

hirbod commented Jul 28, 2024

Pretty odd imo. Header is a sibling of TrueSheet. Magically pinning it to 0,0 is kinda weird to me, just because I passed a scrollRef.

I'll try it again later, but in my tests I wasn't able to make it work correct with padding. Removing the scrollRef works, not sure about the downsides yet, since in this particular case we do not expand the sheet.

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

@hirbod yeah, it needs to be pinned for the sheet to work. It's how IOS's bottom sheet works.... if you remove the pinTo code, the height of the list doesn't update :(

For consistency with android, float your header and add paddingTop to the list. That's what I did :)

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

I tried various solution to the auto height problem and ended up with the layout constraints for IOS. Like adjusting the height of the list when dragging, etc. The constraints code is only the most performant. Thus needing the scrollRef prop.

@hirbod
Copy link
Contributor Author

hirbod commented Jul 28, 2024

Gotcha, will dig deeper later.

@hirbod
Copy link
Contributor Author

hirbod commented Jul 28, 2024

By the way, speaking of FlashList: adding an example for it would be good, too. Specially since getting the ref is a little bit different.

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

I actually didn't know that :D. I'll add it to the docs! Thank you

@hirbod
Copy link
Contributor Author

hirbod commented Jul 28, 2024

FYI, using [500, 'large'] does not work with FlashList, the list does expand but the height is not adjusted.

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

Can you try passing the default ref of the FlashList? Also try to wrap it on a View and pass that view's ref instead.

@hirbod
Copy link
Contributor Author

hirbod commented Jul 28, 2024

Passing the default ref of FlashList is not working. Passing the wrapped ref of the View behaves almost identical.
Might be a FlashList thing though. I guess it needs to re-render.

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-07-28.at.17.45.57.mp4

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

Yeah.. it has something to do with the FlastList's internal. It may have another "container" that's preventing the height constraint to work.

A dirty fix would be to wrap the list on a view with static height :/

const dimensions = useWindowDimensions()
<View style={{ height: dimensions.height - insets.top }}>
  <FlashList />
</View>

@lodev09
Copy link
Owner

lodev09 commented Jul 28, 2024

I have a feeling that this will get better when new arch is already supported. My guess is that FlashList is doing some calculation on the parent but it can't determine the adjusted height since it's resized natively.

@hirbod
Copy link
Contributor Author

hirbod commented Jul 28, 2024

Using useWindowDimensions won't work since the sheet is a "form sheet" and does not take up 100% of the screen. One would need to know the exact dimensions of medium and large detents plus all constraints to figure this out. I'll stick with a fixed height of 500 for now and revisit this later. I've already spent too much time dealing with all the quirks.

@spicy-xili
Copy link

Assume the following structure:

<TrueSheet>
  <MyHeaderComponent />
  <FlashList />
</TrueSheet>

All good so far. But once I pass the scrollRef (with FlashList, unlike FlatList, you need to pass the scrollableNode like so):

ref={(ref) => {
  if (scrollRef) {
    ;(scrollRef.current as unknown) = ref?.getScrollableNode()
  }
}}

It works fine. The main problem I have is that (at least on iOS; I haven't tested Android yet), there is a utility function called pinTo, which now pins the whole view behind MyHeaderComponent, thus not taking the element's height into account.

I've experimented with TrueSheetView and tested adding offsets like so:

rctScrollView.pinTo(view: contentView) { constraints in
    constraints.top?.constant = 50 // 50 is the height of my header
}

So this works. I think there are multiple ways to solve this problem:

  1. Add a HeaderComponent, similar to FooterComponent, and handle all of this under the hood.
  2. Ensure to run through all the siblings of TrueSheet before the actual ScrollView and automatically calculate the offset.
  3. Add a prop (like scrollViewOffset), and we pass a value to it, like 50

Before Patch After Patch
After Patch Before Patch
You may ask why I don't use HeaderComponent by FlashList? Well, because it does not keep the header fixed.

My example above is just a quick "test", not a fix. The offset needs to be reflected in all calculations etc, similar to the footer.

Hi @hirbod, could you give me some hints on how you created the "Add comment" section? I am making a similar "Comments" screen and I have tried putting it into the footer but then I have some problems when trying to use KeyboardAvoidingView.

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

No branches or pull requests

3 participants