lodev09 / react-native-true-sheet

The true native bottom sheet experience 💩
https://sheet.lodev09.com
MIT License
394 stars 12 forks source link

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

Open hirbod opened 1 month ago

hirbod commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

Gotcha, will dig deeper later.

hirbod commented 1 month ago

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 commented 1 month ago

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

hirbod commented 1 month ago

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

lodev09 commented 1 month ago

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 commented 1 month ago

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.

https://github.com/user-attachments/assets/918ebab2-f51c-4d7f-934e-b37ca7ffe5ab

lodev09 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.