Eliav2 / react-xarrows

Draw arrows (or lines) between components in React!
https://codesandbox.io/embed/github/Eliav2/react-xarrows/tree/master/examples?fontsize=14&hidenavigation=1&theme=dark
MIT License
584 stars 75 forks source link

The lines don't move together within their container if overflow #41

Closed oyalhi closed 3 years ago

oyalhi commented 3 years ago

If the container where the lines are drawn has overflow the whole contents move but the lines don't move with the pointed elements.

https://user-images.githubusercontent.com/57917316/104488518-503a3200-5583-11eb-946c-f01a17b2aa36.mov

Eliav2 commented 3 years ago

The arrows should be placed inside the scrollable window and not above it in the dom, else at won't move with the scroll. If you really strugles with this you can install react-xarrows v1.3 and use the property monitor Dom changes which listen to events such scroll

oyalhi commented 3 years ago

All the items you see are inside a single div, which is a flex has 3 divs, 3rd one being the xarrows. I put a border around the container div of the xarrows and can confirm that div itself moves, but the arrows inside it don't.

I have installed v1.3, and used the monitorDOMchanges without success.

The issue happens when inside containers have overflow-y: auto enabled; otherwise works as expected. I will try to create a minimal reproducible example and share it here for your review.

Eliav2 commented 3 years ago

Thank you friend, will be appreciated.

oyalhi commented 3 years ago

Hello. I've created a minimal reproducible example. Please find it here:

https://codesandbox.io/s/gallant-silence-71p8f

Eliav2 commented 3 years ago

very well friend... i have no idea why this is happening... but this is absolutely a bug! if you accidently found out why is this happening let me know!

jithureddy commented 3 years ago

@oyalhi I would have this included in the library. However we can accomplish the scroll behaviour like this example https://codesandbox.io/s/interesting-solomon-b0292

oyalhi commented 3 years ago

@jithureddy very nice workaround and thank you for the input, love it.

After reading the useScroll hook, it mentions:

React sensor hook that re-renders when the scroll position in a DOM element changes.

So essentially all we need to do is add the reference to the scroll div and call the hook:

useScroll(scrollRef)

No need to set the state to force a re-render, the hook does that for us.

Here is your code with removed code lines and a working copy: https://codesandbox.io/s/gallant-kapitsa-hxuhz

jithureddy commented 3 years ago

@oyalhi I am experiencing performance issue at around 150 nodes. Like lines move behind and get connected with nodes after a while.

https://user-images.githubusercontent.com/5911189/110506408-f96e5600-80f6-11eb-8099-13d8f9aab3c6.mov

Also we have working example for the same. In my case I have do bit of computation to place the nodes nicely in grid. Each node is going to have lots of details.

Example - Many nodes

@Eliav2 Please suggest an option for this

oyalhi commented 3 years ago

@jithureddy the above mention topic is not related to lines moving together but a performance issue. It might make sense to open a new issue since they are unrelated.

jithureddy commented 3 years ago

@oyalhi Well, You are right. I will open that as separate issue. However I have css fix for the lines moving together problem. @Eliav2 Was correct if we keep the lines with in scroll container they should move but if only lines are positioned with in its parent.

I have working example for this without the above useScroll work around.

Please check this example.

I have applied relative position style for the scrolling container. By the virtue of DOM properties I don't see performance issue as well.

Eliav2 commented 3 years ago

@jithureddy you are absolutely right. first, I implemented the xarrow in such a way that if you put the xarrow outside a scrollable window and pointed to a connected element inside the scrollable window then I've added eventlistenerts for the scrolling of the window(and more listeners such resize) and calculated the scroll diff and took this diff into account in the calculations.

this was the behavior until react-xarrows v1.4. then I realized this is just not 'reactish' - one of the principles of react is to not touch the DOM directly with eventListeners. the component should know how to calculate things based on the react-tree and not the dom-tree. until v1.4, and to achieve this behavior, each xarrow remembered all parents of the connected elements and xarrow until the common-ancestor and listened to the events of all of them. this was convenient to the user because I could monitor exactly where the xarrow where placed and console some warning when the placement was not recommended, but this was unreactish and inefficient for performance.

this behavior was removed at v1.5. because currently the xarrow doesn't have awareness of the connected elements, users should know in what hierarchy in the DOM to place xarrow in such a way that no rerender of the arrows will be needed if scroll occurs, this is possible most of the time but not always, in cases where this is not possible use the useScroll hook. I need to find time to rewrite sections of this component to more 'reactish' architecture in v2.0. currently, I have no time to do so.

in relation to the performance issue, did you use the useScroll hook? as I said above - if you place the xarrow in the right hierarchy you wouldn't need anymore this hook and this will solve the performance issue.

Eliav2 commented 3 years ago

actually, after relooking at the codesandbox you provided - this is not a bug. this is a design limitation. the xarrows in your example should be inside the scrollable window in order to be scrolled with(or else trigger a render using a scroll hook or manually render the parent using some hook). this is not a bug, but hopefully a better design will be introduced at v2.0 that would overcome these cases

Eliav2 commented 3 years ago

see v2 solution: https://github.com/Eliav2/react-xarrows/issues/75#issuecomment-882135339