ubc-carnap-team / Rudolf

Truth Tree Widget for Carnap
2 stars 3 forks source link

React LineTo lines not rerendering on window resize #63

Closed mbecker20 closed 4 years ago

mbecker20 commented 4 years ago

Naively rerendering all of NodeView to rerender the LineTos fixes the issue, but is too slow and unnessarily rerenders many componenets, and LineTo does not document an individual rerender method. One solution is to switch from using LineTo to react archer, which is newer/more actively developed and structures all the lines in a standalone svg layer apart from the interactive later. More to the point, it has a method for the rerender of the svg layer independantly. The react archer repo is here. I have used this library before, and the downside is the additional boilerplate, as well as some potential svg layer trickiness that could have to be ironed out. There may be an easier way to handle this though, so if any of you @McTano @orbit-stabilizer @kylemas @lf- have any ideas as to how to rerender only the LineTo lines before I emplement this change please share.

McTano commented 4 years ago

I have a couple of questions:

  1. How are you forcing the NodeView components to re-render?
  2. How slow is it to re-render all the NodeViews, and how are you measuring that?

I could comment on the pros/cons of that solution if you open a draft PR with the code.

Generally the way you force something to re-render in React is to pass down a prop which will have changed when a re-render is needed. In this case, passing the window dimensions down to the LineTo component somehow as key might work. Maybe in the class name.

I'd be all for using react-archer instead of LineTo. LineTo is kind of junky and not maintained anyways, and it raises compiler warnings.

McTano commented 4 years ago

Actually, I remembered that the way to force a component to re-render is to pass it a new key prop.

mbecker20 commented 4 years ago

1: I attached a useState to the NodeView component holding window dimensions, and added a callback to resize listener that calls setState passing the window dimensions. 2: It took several seconds to resize for large trees on my desktop, when without the resize callback it was almost instantaneous besides the LineTo not rerendering.

mbecker20 commented 4 years ago

Actually, I remembered that the way to force a component to re-render is to pass it a new key prop.

I think this might be it

mbecker20 commented 4 years ago

how would you pass it a new key prop without rerendering NodeView though? Maybe could use useRef and give all LineTos the refs to update all ref.current.key on resize, but this smells messy.

McTano commented 4 years ago

I just tried passing the stringified dimensions as a key to the component and it works. Doesn't seem to be a speed issue as far as I can tell.

This kind of thing is a bit finicky to do in React, so I can push my code now that I've got it working. Would you mind testing it again with the large tree and seeing if it renders smoothly?

McTano commented 4 years ago

To answer your question, I didn't use any refs to update the lines imperatively. I set the window.onresize listener in the App component and had it update the react store. The prop gets passed down the component tree as a prop. Then, since this is mostly still idiomatic React, React just does whatever magic it normally does and the updates shouldn't be too expensive.

McTano commented 4 years ago

I still think it would be good to replace the react-line-to library, though, because it's ugly.

lineto_is_ugly

Pictured: ugly.

mbecker20 commented 4 years ago

I'll implement react archer then in ui dev 63.