fivecar / react-native-draglist

FlatList that can be reordered by dragging its items
MIT License
119 stars 19 forks source link

Dynamically changing parent component size without re rendering the draglist - breaks the drag and drop behavior (flatWrapLayout.current.y is wrong) #14

Closed OmerGery closed 1 year ago

OmerGery commented 1 year ago

First of all - this library is awesome, works well for me aside from this issue. started using after react-native-draggable-flatlist was randmoly crashing / freezing for me on ios. (probably because of reanmiated issues). The issue is simple - if the react-native-draglist component is being rendered on a component which's size changes (on my use case bottom sheet being closed/opened) - the drag and drop position is not being re-measured. Its a bit of an edge case - there is no way for the draglist to know that such event occured (the data prop didn't change). My proposition is to expose via ref (useImperativeHandle ) a callback to trigger a re-measure of the flatWrapLayout.current https://github.com/fivecar/react-native-draglist/blob/d77ac6aaaf5400ca6b119438c21c49b9e3f40503/src/index.tsx#L254 That way , the parent component can use the ref and trigger it for the y position to be correct. In my use case - the bug is that that the draglist is being rendered on a bottomsheet. Once the bottomsheet is switched from being collapsed to expanded, or the opposite - the drag and drop flatWrapLayout.current.y start position is wrong and the drag does not work as expected. if the draglist is being rendered only after the collapse / expantion finishes so it works good. A workaround i found is to remove the last data item from the data array, and add it again, this triggers the calculation again, and makes the drag work as expected. Ill try work on extracting a video demonstrating the behavior, Thanks!

fivecar commented 1 year ago

@OmerGery : Thanks for taking the time to write such detailed feedback. I really appreciate the energy you put into helping improve this library.

I think I understand what you're asking for (i.e. specifically a way for a parent of the component to force re-measurement of the wrapped FlatList), but I'm not sure I fully understand why the existing code isn't working. You mentioned the parent BottomSheet changing its expand/collapse state... but the <DragList> wraps its FlatList component with a View whose onLayout should in theory force the flatWrap to re-measure whenever a parent changes the DragList's size. Is this not happening? i.e. when your BottomSheet expands/collapses, is the DragList's View not getting onLayout called again?

OmerGery commented 1 year ago

Hey @fivecar thanks for the quick reply, appreciate it So i further investigated according to what you suggested - the onLayout event is called only when the amount of items in the data={data} array is being changed (item added/removed) if a data element is edited, or the bottom sheet is being closed/opened - it is not called.

image

If i had a way to just force trigger it when my bottomsheet opens/closes it would be great

OmerGery commented 1 year ago

Full example showing the triggers of the onLayout event. https://github.com/fivecar/react-native-draglist/assets/68545675/81d7544c-e90f-49c3-bcd9-1ba5d67eabe7 My current workaround is to force manipulate the amount of items in the data array once the bottomsheet is opened.

OmerGery commented 1 year ago

https://github.com/fivecar/react-native-draglist/pull/15#partial-pull-merging :) :)

fivecar commented 1 year ago

@OmerGery : Could you please take a look at https://github.com/fivecar/react-native-draglist/pull/16 and LMK what you think? In that draft PR, all I did was modify the example app to show that whenever a parent view is resized, DragList does actually call its onLayout as expected. In other words, you shouldn't need to mess with the data elements in order to get it to re-layout.

Could you LMK how this differs from what you're experiencing? Or ideally, could you modify that example to reproduce the issue you're having? It seems to me that, at least in the example app, DragList re-lays-out whenever its parent changes size (which should in theory address the core issue you've raised, IIUC).

OmerGery commented 1 year ago

@fivecar I will try to reproduce on the example app in the PR you opened. But please watch the video i attached, it is a full example of the onLayout not being called when bottomsheet is opened/closed. Also, my PR (15) handles it without the need to manipulate data

OmerGery commented 1 year ago

I think the issue with the bottomsheet is that its not only resize, the whole location of the parent element changes

fivecar commented 1 year ago

@OmerGery : please do LMK if you can produce the issue in the example app. It's unclear to me (as yet) why a wrapping View wouldn't get onLayout when its size changes (as shown in the example code in #16). If we could get down to the circumstances under which this is true, it might help find a durable generalized solution to the problem. Thank you for your help!

fivecar commented 1 year ago

@OmerGery : Ah, wait - I totally get what you're saying now. I watched the video, and it makes much more sense -- thanks for providing that. You're asking about cases where the on-screen location of the entire list changes whilst the surrounding View isn't relaid-out (because its size hasn't changed). Totally makes sense to me that this is an issue (and one that needs to be fixed).

I'm a bit flooded today, but will take a look soon. Thanks for being so patient with raising and explaining this issue.

fivecar commented 1 year ago

@OmerGery : Ok, PR https://github.com/fivecar/react-native-draglist/pull/16 now reflects a fix for this issue. Would you mind trying it in your app and letting me know if it does it?

Specifically, because onLayout is only called for resize but not for move, and React Native doesn't give you a way to subscribe to component moves, we now measure the component's position on the screen during panResponderGrant (which is when you start dragging). This way, no matter where the window has moved before you start dragging, we get the y position correctly.

Once you verify that it works in your case, I'll merge and release. This should hopefully fix it for everyone without requiring people to write extra code.

OmerGery commented 1 year ago

@fivecar Cool thanks! I will try #16 it on my app This sounds like a much better solution then exposing the measure function (although it can be good to let it be for any weird cases the lib might have not covered).

fivecar commented 1 year ago

@OmerGery : Any progress on this? If I don't hear back by tomorrow, I'm going to assume it works for you just as well as it did in the example app; I'll then merge and release that change and close out the PRs. LMK!

fivecar commented 1 year ago

Ok - pushed this fix out as version 3.5.1. Thanks for reporting this, @OmerGery, and for suggesting possible fixes!

OmerGery commented 1 year ago

@fivecar Seems to work good. Thanks so much for your time and help! Sorry for the late reply , was super busy

fivecar commented 1 year ago

Thanks for getting back to me, @OmerGery , and for taking the time to report this issue. I really appreciate it!