Flat render thoughts

[raineorshine]

Instead of rendering thoughts hierarchically in the DOM, render them all at the same level.

e.g. Instead of this:

<Thought value="A">
  <Thought value="B">
    <Thought value="x"></Thought>
    <Thought value="y"></Thought>
    <Thought value="z"></Thought>
  </Thought>
</Thought>

We want them rendered in the DOM like this:

<Thought value="A" lvl="0">
<Thought value="B" lvl="1">
<Thought value="x" lvl="2">
<Thought value="y" lvl="2">
<Thought value="z" lvl="2">

Instead of visually hiding ancestors, do not render them at all.

The aim of this task is to eliminate empty space above and below deeply nested thoughts and to increase performance by not rendering ancestors hidden by the autofocus. It will also allow more complex navigation animations.

Extra care must be taken when the cursor moves to ensure that the visible thoughts remain in exactly the same position within the viewport without any visual choppiness. This includes any custom scroll position handling.

This is primarily a refactor. Visually the app should stay nearly identical.

[shresthabijay]

Plan of action for Flat Rendering

Here is the brief plan to implement flat rendering https://docs.google.com/document/d/12g_w-vR9Ygz55ifqH7H47pBMnSeyZnyDfzxUZENKfss/edit

Current progess #503

Tasks accomplished:

1. Create a function that return visual representation of visible thoughts as flat array.
2. Basic UI flat rendering. https://drive.google.com/file/d/1l_KN_Gxa5zihD2Cjb1EVn5vOo_wtV98d/view
3. Add basic animation to the flat rendered thoughts (offset , mount and unmount animation). https://drive.google.com/file/d/1lS5JZfBe2XqP5Jk1KGPzexH2YjHZkIxA/view

Challenges to overcome

1. Child nodes don't know the animation state of the parent nodes. For certain animations when child is mounting it needs to start it's animation based on current animation state of the parent node. In the attached gif you can see, when we select node animation , node test starts to animate to the right. But when we select test immediately, it starts to animate towards left from the midway. But children nodes harry, potter and hermione don't know the current animation state of its parent node test, causing their mount offset animation towards left to lag behind few pixels.

2. Currently animation timeout is dynamically controlled for each nodes based on animation state of Transition component. To avoid animation on exit for some nodes timeout is set to 0 between re-render. But it is not consistent and sometimes component still unmounts with fraction of delay. It causes little visual lag. If you watch the gif example , you can see small visual lag caused certain delay of unmounting nodes.

Animation Gif

3. With current implementation top offset works correctly because we have fixed vertical height occupied by each node. How do we calculate top offset when we have other views like context and table ?

[raineorshine]

To avoid animation on exit for some nodes timeout is set to 0 between re-render. But it is not consistent and sometimes component still unmounts with fraction of delay. It causes little visual lag.

I was thinking about this, and I think we have no choice but to hide the element with CSS instead before unmounting it in order to have a smooth animation. This reminds me of graphics rendering where you render to a hidden back plane and then "flip" it instead of rendering directly to the front plane.

[shresthabijay]

@raineorshine my current plan for unmount animation was to pre-determine all the nodes that should not animate if they unmount on next re-render.

Unmount Animation Cases:

1. Any node that is descendant of previous cursor should not animate if they unmount on next render . If not descendant of cursor it can animate out on unmount.

In this example thoughts n, o and p are descendant of prev cursor l. When cursor changes to k the mentioned thoughts need to unmount without animation.

In this example when cursor changes from l to n, thought k and m needs to unmount . Since they are not descendant of the prev cursor. They need to unmount with animation.

2. In case when cursor is a leaf, rule 1 needs to be updated. For this case cursor itself and any node that is sibling or descendant of cursor should not animate if they unmount on next render . If not they should animate out on unmount.

In this example m is the cursor and is a leaf node. So when cursor changes from m to k. Both m and it's sibling needs to unmount. Since m is prev cursor itself and n is the sibling, so they should not animate on unmount.

3. If next cursor is the RANKED_ROOT i.e navigate to home then there should be no animation at all for any unmounting nodes.

1, 2 has been easily achieved but there was a problem implementing case 3. Since case 3 depends on new cursor instead of previous cursor we cannot pre-determine the animation output for unmounting nodes before hand. So we have to check after every re-render that if the new cursor is a RANKED_ROOT and if it is then we should dynamically stop all the unmount animation. The problem is that the Transition component uses functional rendering and on entering exiting state it doesn't get any updated variable from the HOC including the cursor variable.

Hacky Solution: I used store.getState to get updated cursor inside Transition component to check if the new cursor is a RANKED_ROOT. It works perfectly. But it is a hacky anti pattern solution that kinda breaks the flow the data.

[shresthabijay]

@raineorshine These are the only way we can change the cursor right ?

1. selecting/clicking specific visible thoughts
2. clicking outside (cursor up)
3. navigate to home
4. cursor up
5. cursor down

Please let me know if I am missing something!

[raineorshine]

There are the movement actions that you listed, which also include cursorNext, cursorPrev, cursorBack, and cursorForward. There are also any of the editing actions which may move the cursor in arbitrary ways, for example subcategorizeOne, bindContext, bumpThoughtDown etc. Basically anything that sets the cursor in their reducer or dispatches setCursor.

We will need a general solution. It will not be viable to individually test the different actions that result in cursor changes. You can however diff the old cursor and new cursor and use that to determine how the cursor is moving.

[shresthabijay]

@raineorshine It is clear that we cannot implement animations by predetermining before hand like my previous implementation. Let's land on a proper plan that we both agree on before we invest more time on this task.

I have a plan in mind. Instead of just creating flat array of visible thoughts, we create flat array of all the invisible thoughts starting from the root and stop at the deepest visible nodes.

The advantages are :

1. We don't have to use TransitionGroup because we won't need to actually unmount invisible nodes.
2. We don't have to tackle challenge 3

   With current implementation top offset works correctly because we have fixed vertical height occupied by each node. How do we calculate top offset when we have other views like context and table ?

   It is because all the invisble thoughts would actually be available in the DOM. So calulating top offsets won't be necessary at all. This is one of the biggest challenge we have.

But the real question is why do we render all the unnecessary nodes that are invisible? They can cause performance issue because they have expensive render cycle and may re-render on cursor change. Since the main focus of this task is performance optimization we can't afford that.

To address this issue we don't actually render Thought component when node is invisible. Instead we just render an empty div with proper height. So even if nodes re-render , the performance cost of the application would only be affected by visible nodes that render Thought component.

Please let me know what you think!

[raineorshine]

I have a plan in mind. Instead of just creating flat array of visible thoughts, we create flat array of all the invisible thoughts starting from the root and stop at the deepest visible nodes.

This sounds like a hybrid of the old rendering and the desired flat rendering. That's not what we want. We want clean rendering pipeline that calculates offsets rather than using dummy elements to fill space.

To address this issue we don't actually render Thought component when node is invisible. Instead we just render an empty div with proper height.

Why render an empty div for each Thought, instead of calculating the top offset? The latter is cleaner and gives us more control. Keep in mind that the vertical space is meant to be intelligently collapsed in the new rendering approach. We have to maintain vertical position invariants without allowing a lot of empty space to build up like in the current app.

[raineorshine]

1. Any node that is descendant of previous cursor should not animate if they unmount on next render . If not descendant of cursor it can animate out on unmount.

In this example thoughts n, o and p are descendant of prev cursor l. When cursor changes to k the mentioned thoughts need to unmount without animation.

The production app likely will have an animation for this transition. Perhaps a very fast wipe up. m should animate its vertical offset so that it does not overlap with the unmounting thoughts, but instead follows them up. We don't have to know the exact animation now, but we do need to build in the capacity for m to animate in without overlapping the thoughts animating out.

In this example when cursor changes from l to n, thought k and m needs to unmount . Since they are not descendant of the prev cursor. They need to unmount with animation.

Yes, that's right. And it may potentially be a different animation than others.

2. In case when cursor is a leaf, rule 1 needs to be updated. For this case cursor itself and any node that is sibling or descendant of cursor should not animate if they unmount on next render . If not they should animate out on unmount.

In this example m is the cursor and is a leaf node. So when cursor changes from m to k. Both m and it's sibling needs to unmount. Since m is prev cursor itself and n is the sibling, so they should not animate on unmount.

Yes, although I assume that would not be a special case, but would be subsumed under the rule that "cursor descendants should not animate on unmount."

3. If next cursor is the RANKED_ROOT i.e navigate to home then there should be no animation at all for any unmounting nodes.

Yes. This falls under the same rule as above, since all thoughts are descendants of the root.

1, 2 has been easily achieved but there was a problem implementing case 3. Since case 3 depends on new cursor instead of previous cursor we cannot pre-determine the animation output for unmounting nodes before hand.

All animations depend on the new cursor I believe.

So we have to check after every re-render that if the new cursor is a RANKED_ROOT and if it is then we should dynamically stop all the unmount animation. The problem is that the Transition component uses functional rendering and on entering exiting state it doesn't get any updated variable from the HOC including the cursor variable.

It doesn't have to be after the re-render, just after the state has been updated with the new cursor. The unmount itself is not triggered until the state has changed, so presumably we have access to the new cursor at that point.

Hacky Solution: I used store.getState to get updated cursor inside Transition component to check if the new cursor is a RANKED_ROOT. It works perfectly. But it is a hacky anti pattern solution that kinda breaks the flow the data.

I don't think checking specifically for the root is correct, but I do think that we will need to access the state directly from the Transition component in order for it to know how to animate.

[raineorshine]

Let me know if that is enough feedback for you to take the next step towards a plan, or if you need more from me! Thanks.

[shresthabijay]

Why render an empty div for each Thought, instead of calculating the top offset? The latter is cleaner and gives us more control. Keep in mind that the vertical space is meant to be intelligently collapsed in the new rendering approach. We have to maintain vertical position invariants without allowing a lot of empty space to build up like in the current app.

Yes I agree. But with absolute positioning comes lot of challenges for calculating vertical offsets. It is easy to calculate offset if nodes always have constant height. But as you can see the attached examples . At such cases we cannot calculate proper vertical offset for node c and node sas before nodes above them render.

Instead of making all the visible thoughts absolutely positioned we can wrap them in a div and absolutely position the main wrapper with vertical offset based on total number of invisible thoughts above them. That way we can tackle problem of calculating vertical offsets within visible thoughts div and can leave empty space above. But with that we need a way to properly time change of top offset of wrapper with completion of unmounting animations. What do you think ?

[raineorshine]

Yes I agree. But with absolute positioning comes lot of challenges for calculating vertical offsets. It is easy to calculate offset if nodes always have constant height. But as you can see the attached examples . At such cases we cannot calculate proper vertical offset for node c and node sas before nodes above them render.

Not sure what I was thinking before... we should not be absolutely positioning the thoughts vertically. We can leave that to the DOM. We should just be absolutely positioning the horizontal position of the thoughts.

The only except is that when the user navigates, thoughts will be unmounted and would abruptly change the vertical position. In order to keep this smooth, we will need to animate the vertical position back into place. I don't believe this will require per-thought positioning though. We will just need to know the vertical offset from the transition.

Instead of making all the visible thoughts absolutely positioned we can wrap them in a div and absolutely position the main wrapper with vertical offset based on total number of invisible thoughts above them.

Thoughts that are not rendered should not take up any space, so I'm not sure I understand the motivation here.

[shresthabijay]

Thoughts that are not rendered should not take up any space, so I'm not sure I understand the motivation here.

Don't we need to maintain top offset as we go deeper into tree?

[raineorshine]

The aim of this task is to eliminate empty space above and below deeply nested thoughts

We do not want to maintain the top offset. This was stated clearly in the original issue and in a PR comment.

The content should slide up as thoughts are unmounted.

[shresthabijay]

The aim of this task is to eliminate empty space above and below deeply nested thoughts

We do not want to maintain the top offset. This was stated clearly in the original issue and in a PR comment.

The content should slide up as thoughts are unmounted.

Ah. I just got it. I was in misconception that we wanted to maintain top offset as we kept going deeper. Sorry my bad. Things just got lot easier. Phew!

[raineorshine]

Hi Raine! Updates from my side. I am working on animations for flat rendering. I refactored code from calculating offset before hand to calculating offsets and animation variable comparing difference between previous and current flatArray.

Also TransitionGroupfunctional rendering approach didn't allowed access to updated variables within the scope for unmounting Transition components. So I invested my time today to work with react-spring because it allowed using hooks which solves the problem we are having with TransitionGroup.

There is useTransition hook that does the list unmount/mount animation just like TransitionGroup but with much more flexibility. I tried basic mount and unmount animations and they worked great. But soon I found a specific issue with the library that will prevent us from using it. When multiple nodes are deleted at a time, the ordering of the list doesn't remain consistent.

Calculating x offsets only, correct?

Also, can you explain why you need to diff the flatArray? I was thinking that you only need to know where the new cursor is to know what kind of unmount is happening.

Also TransitionGroupfunctional rendering approach didn't allowed access to updated variables within the scope for unmounting Transition components.

What about the onExiting handler? You can access state directly from there when the component is unmounting.

Apparently there was a PR to this issue by the react-spring community but later it was reverted due to regression it introduced. https://github.com/react-spring/react-spring/pull/605

Looks like it was again fixed #626 (https://github.com/react-spring/react-spring/pull/626) a year ago and is waiting for v9 to be released (https://github.com/react-spring/react-spring/pull/632).

We can always add the branch as a dependencies until it gets released officially.

[shresthabijay]

Also, can you explain why you need to diff the flatArray? I was thinking that you only need to know where the new cursor is to know what kind of unmount is happening.

When the user navigates, thoughts will be unmounted and would abruptly change the vertical position. To keep it smooth we need to animate the vertical position by the number of thoughts above that are unmounting. To do that we need to calculate all unmoutning nodes that are above new cursor. Also when thoughts are added above , we may want similar animation so that it looks smooth. So for this reason we need to find difference between previous and new flat array.

[shresthabijay]

What about the onExiting handler? You can access state directly from there when the component is unmounting.

There is a props that we can pass called onDestroyed that is supposed to be called when thought unmounts from the list. I haven't tried it yet. I will let you know after I try it.

[shresthabijay]

Looks like it was again fixed #626 (react-spring/react-spring#626) a year ago and is waiting for v9 to be released (react-spring/react-spring#632).

We can always add the branch as a dependencies until it gets released officially.

That's awesome. I was really excited to use react-spring. I will try this specific branch and get back to you!

[raineorshine]

Also, can you explain why you need to diff the flatArray? I was thinking that you only need to know where the new cursor is to know what kind of unmount is happening.

When the user navigates, thoughts will be unmounted and would abruptly change the vertical position.

Yes

To keep it smooth we need to animate the vertical position by the number of thoughts above that are unmounting.

Not just the number, but the heights, as they may vary.

To do that we need to calculate all unmoutning nodes that are above new cursor. Also when thoughts are added above , we may want similar animation so that it looks smooth. So for this reason we need to find difference between previous and new flat array.

Okay, I think that makes sense.

[raineorshine]

What about the onExiting handler? You can access state directly from there when the component is unmounting.

There is a props that we can pass called onDestroyed that is supposed to be called when thought unmounts from the list. I haven't tried it yet. I will let you know after I try it.

I don't see onDestroyed. I just see onExit, onExiting, and onExited. http://reactcommunity.org/react-transition-group/transition

[shresthabijay]

I don't see onDestroyed. I just see onExit, onExiting, and onExited. http://reactcommunity.org/react-transition-group/transition

I was addressing the react-spring library useTransition hook. Are we still planning to use react-transition-group ? https://www.react-spring.io/docs/hooks/use-transition

[raineorshine]

I don't see onDestroyed. I just see onExit, onExiting, and onExited. http://reactcommunity.org/react-transition-group/transition

I was addressing the react-spring library useTransition hook. Are we still planning to use react-transition-group ? https://www.react-spring.io/docs/hooks/use-transition

Ah, I thought we were still talking about react-transition-group. No, go ahead with react-spring.

[shresthabijay]

@raineorshine react-spring@v9 is working properly as expected. I worked on recreating all the animations using springs. It's far more flexible and easy than previous implementation. I am using one controller for list animations and another controller to animate wrapper div accordingly to the number of thoughts that are added or deleted above. But I am not being able to time two different animations properly.

After thought is deleted above I animate using translateY to move whole div to move up smoothly. And once list animation is complete and thoughts are actually unmounted, we need to set translateY back to zero. But it's being quite tricky to time this because springs are not time based. You can see in the gif at the end it animates smoothly but then somewhere between unmounting and setting translateY back to zero timing goes off. I will get back to you with updates.

[shresthabijay]

@raineorshine Instead of using transformY to animate the wrapper div when thoughts are unmounting, I decided to animate the height of the unmounting nodes. It was tricky but it looks good. I have attached video link to show the latest update. There are still many things to adjust and reconsider. Let me know what you think about this. Also let's discuss and finalize how we want the all animations to look.

Latest update https://www.loom.com/share/dae94eae5478485db50b6b24704a60bc

[raineorshine]

Nice work! This looks great!

Animating the vertical position is a big change, so we don't know yet if it will feel smooth or jarring for the user. I think we have to experience it directly to know. I can say that the video looks good, and doesn't strike me as jarring. The animation helps you follow the change, even when the thoughts are moving around a lot. Seeing it in action with the cursor will be the real test though.

The other idea I thought of was to preserve the vertical position and adjust the body scroll to prevent lots of white-space from building up. I'm not sure if that could be done smoothly though. It wouldn't be animated, but the abrupt scroll jump might cause a render artifact.

[raineorshine]

Also let's discuss and finalize how we want the all animations to look.

I think re-creating the existing app and doing basic animations to start with is fine. I will eventually hire an animator to design the production animations. We should address that in a separate issue. The important point is that the infrastructure is there to support different animations. Flat rendering will be much more flexible, since we can remove a parent without removing its descendants.

[shresthabijay]

. The animation helps you follow the change, even when the thoughts are moving around a lot. Seeing it in action with the cursor will be the real test though.

Yes I agree! For now you can pull the draft PR to see latest progress in action in your local system.

[shresthabijay]

The important point is that the infrastructure is there to support different animations.

Absolutely! I will keep working to complete the infrastructure.

[shresthabijay]

@raineorshine I was looking for similarities between expandThoughts and treeToFlatArray. They are very similar. Infact we need to reuse same expandThoughts logic for treeToFlatArray to check pinned context, table view, isOnlyChildNoURL etc. But the main difference lies in the data structure returned by these functions and also its usage.

expandThoughts returns an expansion map marking all contexts that should be expanded as a key value pair. And as per my observation expanded is used inside Subthought to easily determine if it should render it's children. Please correct me if I am wrong about its use.

treeToFlatArray needs to return all the thoughts that should be visible. In this process it will need logic to calculate nodes for context view , table view too. Also with flat rendering all the visible nodes knows their expansion state unlike current implementation that needs expanded to check if it needs render it's children.

I think we will need to reuse logic from expandThoughts but still create separate treeToFlatArray. What are your thoughts on this ?

[raineorshine]

I think that is a very good assessment. Let's continue with treeToFlatArray and we can look for code reuse afterwards.

It's also possible that treeToFlatArray could be used to generate the expansion map in the same pass. I wouldn't worry about that now though.

[shresthabijay]

@raineorshine Just came across a react-spring@v9 issue where useTransition doesn't update old item in the list. https://github.com/react-spring/react-spring/issues/676 . This could be a problem. Maybe we can find some workaround this.

[raineorshine]

Yes, though it's hard to speculate on before we have a concrete case where it's breaking for us.