Flat render #503

shresthabijay · 2020-04-03T12:02:48Z

Issue: #188

@raineorshine This is a draft PR for the first iteration of flat rendering implementation. There are many challenges as we move forward to create recreate smooth animations and styles. We may need to create custom HOC to tackle some of the animation challenges. Please test the current implementation. It is far from being complete and lacks many details rn but still we are making good progress.

I have used right split pane to render the FlatTreeRenderer . You can open the split view and see the renderer in action while playing around with current recursive implementation on the left side of the split view.

raineorshine

How much overlap is there between getFlatArray and expandThoughts? Do you plan on consolidating those functions?

src/components/FlatTreeRenderer.js

shresthabijay · 2020-04-05T16:16:27Z

@raineorshine I tried your suggested solution of passing onExit prop but it didn't work as expected. I have just pushed a small change to address unmount animation delay issue. I have checked the condition and set exit style to have display property set to none instead of setting opacity to 0. I feel like it works much better than before. But I can't decide on my own if that is an improvement. Would you please pull the latest code and test it ? Just create few thoughts and populate one of them with at least 5 to 10 subthoughts and see if direct unmount (without animation) happens without any significant delay as before.

raineorshine · 2020-04-05T18:31:07Z

3470495 is doing the same thing it was doing before; subthoughts fade out and overlap distant cousins.

https://youtu.be/DI5w9kJSrb0

I tried your suggested solution of passing onExit prop but it didn't work as expected.

In what way? Was it targeting the wrong thoughts, or was the technique not working?

shresthabijay · 2020-04-06T04:47:07Z

3470495 is doing the same thing it was doing before; subthoughts fade out and overlap distant cousins.

https://youtu.be/DI5w9kJSrb0

I tried your suggested solution of passing onExit prop but it didn't work as expected.

In what way? Was it targeting the wrong thoughts, or was the technique not working?

@raineorshine Your observation is absolutely correct. The logic to differentiate unmounting nodes which should animate is still in progress. The issues I wanted to address was that the nodes which directly unmounts without animation still had some delay and lagged.

You can see in the above gif that when cursor is set from thought k to thought a , all the subthoughts of k directly unmounts without any animation. That is the correct behavior but the issue is that they have certain delay before unmount. It is causing visual choppiness as you can see in the gif.

But the problem seems to be solved if we set display to none when node starts exiting.

shresthabijay · 2020-04-06T15:32:00Z

@raineorshine I have pushed latest changes. I have have refactored getFlatArray and completed unmount animation. I have written in detail about the implementation #188 (comment)

There are several cases to achieve unmount animation perfectly. I implemented all of them except case 3. Please read the implementation detail. #188 (comment)

3470495 is doing the same thing it was doing before; subthoughts fade out and overlap distant cousins.

The issue you mentioned above is exactly the third case. With this PR I used very hacky solution to solve it. But I need you to take a look and tell me what you think about it.

Please review it thanks!

raineorshine · 2020-04-06T15:49:45Z

Ah, that it explains it. I tried the one case that you hadn't implemented yet! I now am trying a different case and it is working.

With this PR I used very hacky solution to solve it.

I'm not sure it solved it. Here's a case that does not work:

apple
- banana
  - clementine [cursor]
durian

Activate cursorDown from clementine and the subthoughts of apple incorrectly fade out.

But I need you to take a look and tell me what you think about it.

Will do!

shresthabijay · 2020-04-06T16:08:58Z

I'm not sure it solved it. Here's a case that does not work:

apple

banana

clementine [cursor]

durian

Activate cursorDown from clementine and the subthoughts of apple incorrectly fade out.

Ah, I forgot to consider that we can set cursor to thoughts that are not visible too. I will look at it.

shresthabijay · 2020-04-16T12:46:32Z

@raineorshine With this latest change you can now test the flat rendered tree with cursor action. I have recreated UI to match current implementation to make comparison easier. There are still many challenges to overcome before we reach our goal. I want you to review this to be sure that we are moving ahead in the right direction.

shresthabijay · 2020-04-21T12:52:31Z

Updates

Hide meta thoughts and nodes with =hidden attribute.
Show all hidden and meta thoughts if showHiddenThoughts is true.
Calculate the number of descendant visible nodes that a thought can render if expanded. Use that number to show either ▸ or • icon.
Refactored flatToArray to get recursive updates from children to reduce redundant logic and iteration of children array within a parent scope.

raineorshine

Very pleased with the result so far! The animation looks quite good. It's a little choppy on mobile Safari, but we can address that separately, or worst case disable the height animation on mobile. I don't see any of the major rendering issues we were having earlier. Thank you.

I took a more in-depth look at the code this time. It's looking very good. The component is quite small, and getFlatArray mostly consists of expansion logic, which is expected. I didn't go over the expansion logic in detail, but focused on the recursion and overall architecture. Questions are below.

I renamed some variables and made a couple small refactors. Make sure to pull my changes into your branch.

Thank you!

src/components/Content.js

src/util/flatRender.js

raineorshine · 2020-04-21T22:53:23Z

src/util/flatRender.js

+  })
+}
+
+export const treeToFlatArray = (cursor, showHiddenThoughts) => {


We have a convention to name the file the same as the function when there is a single export.

@raineorshine Sure I will change the name.

src/util/flatRender.js

shresthabijay · 2020-05-01T14:05:30Z

Table View Update

@raineorshine This is the first iteration of table view with flat renderer. Following goals have been achieved:

Recreated 1st column and 2nd column layout without nested nodes. It looks exactly like current recursive implementation.
Collapse 2nd column nodes even if they have single children.
Proper animation.

Here are some clips showcasing the update

There are still many things to work on this before it is fully functional. Will keep you updated!

raineorshine · 2020-05-05T16:54:14Z

Table View Update

@raineorshine This is the first iteration of table view with flat renderer. Following goals have been achieved:

Recreated 1st column and 2nd column layout without nested nodes. It looks exactly like current recursive implementation.

Collapse 2nd column nodes even if they have single children.

Proper animation.

Awesome!!! That is fantastic. Excellent work!

Here are some clips showcasing the update

Table view toggle

https://drive.google.com/file/d/1LQCcGARD1WH9Ov3mQOiolOm0D8BrqDcl/view?usp=sharing

Nice! This looks great. There is a slight modification needed for information design reasons. Take a look at this screenshot of the middle of the animation:

The way column 2 animates in results in a misalignment between thoughts and subthoughts. ii is a child of c, ty, and ola are subthoughts of td, etc, yet the "rows" have been broken and subthoughts are not lining up horizontally until the animation completes. Maintaining the rows is an important design invariant. To do this, we will need the subthoughts to start at the correct vertical level and fade in and translate from left-to-right.

It may be trickier to maintain the row when there are multiple subthoughts in column 2. In this case, the vertical height will be animating to fit the subthoughts. During that time, we have to make sure the subthoughts are animated in at a similar pace so that they do not overlap the row of their uncle in column 1. Not sure how much this requirement will be met by default, or if it will require a lot of custom logic.

Table view toggle with meta =pinChildren

https://drive.google.com/file/d/1LLu4X97f6G_tqnrLisZaJd-J7gFYpn80/view?usp=sharing

WOW. I am stunned by this animation. It looks SO GOOD!!! I am so excited. I'm praying we can get it to run smoothly on all platforms, because this is exactly what we want.

Navigating with table view ON

https://drive.google.com/file/d/1LNLIL8SlYvXiFnaF45foffvOad-v2ZvU/view?usp=sharing

Again this looks absolutely amazing.

There are still many things to work on this before it is fully functional. Will keep you updated!

I haven't looked at the code yet since you're still working on it, but let me know whenever it's ready for a code review, or if you want my advice on any architectural decisions!

Keep up the great work!

shresthabijay · 2020-05-11T16:14:34Z

TABLE VIEW SECOND UPDATE

@raineorshine With this update the table view is now functional to be tested. We have achieved following goals:

Table View second column left to right xOffset and opacity transition on component enter and vice versa on component leave.
Prevent Table View second column yOffset animation on enter.
Allow multiple table view to open.
Adjust nodes at deeper depth to adjust their xOffset according to the number of open table views above them.
Allow changing offset values of first column and second column whenever needed.
Properly grouped and commented logic for code readability.

Table view toggle with left to right xOffset transform and opacity transition.

Table view toggle with one of the children pinned open.

Multiple table views open

Multiple table views with hidden thoughts on

Please review the code and let me know about your thoughts on the progress.

shresthabijay · 2020-05-11T16:24:02Z

TO DO

Since second column uses yOffset transform to be at the same vertical place as first column, it trades off height animation. So when both first column and second column animates out, all second column thoughts align at same position.
The current implementation gives first column a fixed width. So we need to handle text wrap. Right now longer text just spans out of the div.

raineorshine · 2020-05-11T20:38:04Z

The screenshots really speak for themselves. Wow. I think this is going to blow people away. I mean, is there any outliner out there with animations like this?!? It's just incredible.

The table view is looking great and there are more cases than I can name in which animations are working perfectly. We may want to tweak the animations for certain actions, like existingThoughtMove, but we can and should defer that to a future task if it's working adequately as-is.

The only issue I'm noticing is when the cursor moves to column 2 of a table, it reverts to normal view. Instead, it should continue to render column 1 and column 2 as a table, just translated to the left (as in dev).

Unrelated to tables, there's a couple core pieces of functionality that need to be restored, as you may be aware of:

editing
ThoughtAnnotation (with Superscript)
Drag and Drop

Since second column uses yOffset transform to be at the same vertical place as first column, it trades off height animation. So when both first column and second column animates out, all second column thoughts align at same position.

Do you have steps to reproduce? I'm not seeing the issue.

The current implementation gives first column a fixed width. So we need to handle text wrap. Right now longer text just spans out of the div.

Yes, the cells should also be vertically aligned top:

Separate component for drop end convert treeToFlatArray and FlatTreeRenderer to tsx

fix DND root context drop bug

change showContexts to showContextsParent add id to ranked root fix glitch related to no children context component

remove layout animation due to width change issue bind height change to the wrapper div to allow drop end height to cause height animation remove homelink from thought annotation

shresthabijay · 2020-08-29T07:29:05Z

cursorUp is not working. I'm not sure why this is the case, since this reducer shouldn't be affected by rendering.

It has not been working for me in dev for quite a while. I have not been able to find the cause of it.

shresthabijay · 2020-08-29T07:30:41Z

moveThoughtUp/Down is not animated. Wasn't this previously solved by framer-motion?

Yes. It was working. It didn't work well when changing width of the thought for first column width in table view. I have disabled it for now. I will be working on solving that issue.

rebase to dev split and merge content editable behavior fixed

raineorshine · 2020-08-29T19:18:03Z

cursorUp is not working. I'm not sure why this is the case, since this reducer shouldn't be affected by rendering.

It has not been working for me in dev for quite a while. I have not been able to find the cause of it.

cursorUp is working for me in dev. The cursorUp reducer tests are passing in this PR, so maybe the issue is in the action-creator or shortcut.

moveThoughtUp/Down is not animated. Wasn't this previously solved by framer-motion?

Yes. It was working. It didn't work well when changing width of the thought for first column width in table view. I have disabled it for now. I will be working on solving that issue.

Sounds good.

raineorshine · 2020-08-29T19:19:26Z

Fixing the unit tests should probably be the top priority.

use react-use-measure instead of custom hook cancel debounced height callback function on unmount set cursor offset

add id to the cursor on new thought add id to cursor on exisitingThoughtChange find editable using key of the node

add blue overlay on bullet when drag is active update ContentEditable for proper split behavior

shresthabijay · 2020-09-03T08:50:33Z

Fixing the unit tests should probably be the top priority.

@raineorshine I have fixed all the tests. But I ran into problems with drag and drop tests. Turns out testing DND with useDrag and useDrop hooks has not been addressed yet. react-dnd/react-dnd#1540 . We should either use HOC like before or create mock for these hooks. Ideally we shouldn't need to mock these hooks. What do you think ?

shresthabijay · 2020-09-03T08:57:33Z

cursorUp is working for me in dev. The cursorUp reducer tests are passing in this PR, so maybe the issue is in the action-creator or shortcut.

There is a extra white space after px in this file. This is causing split to not return the intended result. Removing that white space fixes cursorUp behavior. Can you confirm that the extra white space is not intended ?

em/src/util/getElementPaddings.ts

Lines 1 to 6 in 9c2bf2c

    
           /** Gets the padding of an element as an array of numbers. */ 
        
           export const getElementPaddings = (element: HTMLElement): number[] => 
        
             window 
        
               .getComputedStyle(element, null) 
        
               .getPropertyValue('padding') 
        
               .split('px ')

raineorshine · 2020-09-03T12:26:43Z

@raineorshine I have fixed all the tests. But I ran into problems with drag and drop tests. Turns out testing DND with useDrag and useDrop hooks has not been addressed yet. react-dnd/react-dnd#1540 . We should either use HOC like before or create mock for these hooks. Ideally we shouldn't need to mock these hooks. What do you think ?

That's a bummer. Does their latest version support HOC, or would we have to decrease the version we are using? I think HOC with tests is probably better than hooks without tests if we had to choose.

There is a extra white space after px in this file. This is causing split to not return the intended result. Removing that white space fixes cursorUp behavior. Can you confirm that the extra white space is not intended ?

Assuming the padding value is 5px 10px for example, then .split('px ') will correctly return ['5px', '10px']. Maybe it's not the whitespace that's the problem, but the way it handles a certain padding value, or no value?

shresthabijay · 2020-09-06T04:20:14Z

Assuming the padding value is 5px 10px for example, then .split('px ') will correctly return ['5px', '10px']. Maybe it's not the whitespace that's the problem, but the way it handles a certain padding value, or no value?

Sometimes padding value can return just 5px. At this case split('px ') will fail. This logic is used for getting padding top inside ContentEditable. Instead of getting padding we can get padding-top which will always be returned with a single value.

update drag and drop tests update component tests fix move thought up add id to the cursor

raineorshine · 2021-06-28T14:41:54Z

@shresthabijay Should we close this and reconstruct it at a later point? It seems too far behind to use this PR.

shresthabijay · 2021-06-29T04:51:20Z

@shresthabijay Should we close this and reconstruct it at a later point? It seems too far behind to use this PR.

Yes. This PR is too far behind. We can use it as a reference later if we need it.

shresthabijay mentioned this pull request Apr 3, 2020

Flat render thoughts #188

Closed

raineorshine reviewed Apr 3, 2020

View reviewed changes

src/components/FlatTreeRenderer.js Outdated Show resolved Hide resolved

raineorshine reviewed Apr 3, 2020

View reviewed changes

src/components/FlatTreeRenderer.js Outdated Show resolved Hide resolved

raineorshine force-pushed the dev branch from a6599c8 to e9d4b92 Compare April 3, 2020 21:39

shresthabijay force-pushed the flat-render branch from 14579de to 3470495 Compare April 5, 2020 15:27

shresthabijay force-pushed the flat-render branch from 3470495 to dd4b96d Compare April 6, 2020 13:55

shresthabijay force-pushed the flat-render branch 2 times, most recently from e56379b to d23b965 Compare April 16, 2020 12:24

shresthabijay force-pushed the flat-render branch from 61dc9e6 to b1a9f73 Compare April 21, 2020 12:42

raineorshine requested changes Apr 21, 2020

View reviewed changes

shresthabijay force-pushed the flat-render branch 2 times, most recently from 1c2b807 to 6db25a1 Compare April 25, 2020 06:26

This was referenced Apr 28, 2020

Scroll to visible content #446

Closed

Table descendants are indented too far #471

Closed

Sort thought during editing #540

Closed

raineorshine mentioned this pull request May 7, 2020

add setCursor on change in Editable #637

Closed

shresthabijay force-pushed the flat-render branch from 4e36f83 to 193dc50 Compare May 11, 2020 15:37

shresthabijay added 6 commits August 29, 2020 13:01

drop end for last child in a context

42df49e

add Drop End Group

0554140

Separate component for drop end convert treeToFlatArray and FlatTreeRenderer to tsx

ThoughtsNew to tsx

bfef8f1

fix DND root context drop bug

return id from rankThoughtsFirstMatch

543790c

change showContexts to showContextsParent add id to ranked root fix glitch related to no children context component

hide bullet on table view

8826574

remove layout animation due to width change issue bind height change to the wrapper div to allow drop end height to cause height animation remove homelink from thought annotation

fix drop end not visible after height animation is complete

5c01516

prevent cursor selection into leaving nodes

11158d9

rebase to dev split and merge content editable behavior fixed

shresthabijay force-pushed the flat-render branch from 15eaaa1 to 11158d9 Compare August 29, 2020 11:31

refactored subthought and thought test for flat-render

91197e8

use react-use-measure instead of custom hook cancel debounced height callback function on unmount set cursor offset

shresthabijay force-pushed the flat-render branch from 55661f3 to 91197e8 Compare September 2, 2020 09:18

shresthabijay added 2 commits September 3, 2020 09:14

add setNoteFocusThoughtId state for note focus at initial mount

3b01e4f

add id to the cursor on new thought add id to cursor on exisitingThoughtChange find editable using key of the node

check if cursor is a leaf by calcualting visible thoughts

cac81f0

add blue overlay on bullet when drag is active update ContentEditable for proper split behavior

change hooks to hoc for react-dnd

dc5c236

update drag and drop tests update component tests fix move thought up add id to the cursor

raineorshine mentioned this pull request Sep 16, 2020

Prevent line break between emoji and word in table col1 #517

Closed

raineorshine force-pushed the dev branch from 2100fab to 657bdd4 Compare November 22, 2020 19:27

raineorshine force-pushed the dev branch 2 times, most recently from 03e5365 to f69d008 Compare January 30, 2021 23:50

raineorshine force-pushed the dev branch from 102e5aa to 882306a Compare March 1, 2021 16:32

shresthabijay closed this Jun 29, 2021

raineorshine mentioned this pull request Oct 26, 2022

Flat Render v2 #1663

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flat render #503

Flat render #503

shresthabijay commented Apr 3, 2020 •

edited by raineorshine

Loading

raineorshine left a comment

shresthabijay commented Apr 5, 2020

raineorshine commented Apr 5, 2020

shresthabijay commented Apr 6, 2020

shresthabijay commented Apr 6, 2020 •

edited

Loading

raineorshine commented Apr 6, 2020

shresthabijay commented Apr 6, 2020 •

edited

Loading

shresthabijay commented Apr 16, 2020

shresthabijay commented Apr 21, 2020 •

edited

Loading

raineorshine left a comment

raineorshine Apr 21, 2020

shresthabijay Apr 22, 2020

shresthabijay commented May 1, 2020

raineorshine commented May 5, 2020

Table View Update

Table view toggle

Table view toggle with meta `=pinChildren`

Navigating with table view ON

shresthabijay commented May 11, 2020

shresthabijay commented May 11, 2020 •

edited

Loading

raineorshine commented May 11, 2020 •

edited

Loading

shresthabijay commented Aug 29, 2020

shresthabijay commented Aug 29, 2020

raineorshine commented Aug 29, 2020

raineorshine commented Aug 29, 2020

shresthabijay commented Sep 3, 2020

shresthabijay commented Sep 3, 2020

raineorshine commented Sep 3, 2020

shresthabijay commented Sep 6, 2020

raineorshine commented Jun 28, 2021

shresthabijay commented Jun 29, 2021

Flat render #503

Flat render #503

Conversation

shresthabijay commented Apr 3, 2020 • edited by raineorshine Loading

raineorshine left a comment

Choose a reason for hiding this comment

shresthabijay commented Apr 5, 2020

raineorshine commented Apr 5, 2020

shresthabijay commented Apr 6, 2020

shresthabijay commented Apr 6, 2020 • edited Loading

raineorshine commented Apr 6, 2020

shresthabijay commented Apr 6, 2020 • edited Loading

shresthabijay commented Apr 16, 2020

shresthabijay commented Apr 21, 2020 • edited Loading

Updates

raineorshine left a comment

Choose a reason for hiding this comment

raineorshine Apr 21, 2020

Choose a reason for hiding this comment

shresthabijay Apr 22, 2020

Choose a reason for hiding this comment

shresthabijay commented May 1, 2020

Table View Update

Table view toggle

Table view toggle with meta =pinChildren

Navigating with table view ON

raineorshine commented May 5, 2020

Table View Update

Table view toggle

Table view toggle with meta =pinChildren

Navigating with table view ON

shresthabijay commented May 11, 2020

TABLE VIEW SECOND UPDATE

Table view toggle with left to right xOffset transform and opacity transition.

Table view toggle with one of the children pinned open.

Multiple table views open

Multiple table views with hidden thoughts on

shresthabijay commented May 11, 2020 • edited Loading

TO DO

raineorshine commented May 11, 2020 • edited Loading

shresthabijay commented Aug 29, 2020

shresthabijay commented Aug 29, 2020

raineorshine commented Aug 29, 2020

raineorshine commented Aug 29, 2020

shresthabijay commented Sep 3, 2020

shresthabijay commented Sep 3, 2020

raineorshine commented Sep 3, 2020

shresthabijay commented Sep 6, 2020

raineorshine commented Jun 28, 2021

shresthabijay commented Jun 29, 2021

shresthabijay commented Apr 3, 2020 •

edited by raineorshine

Loading

shresthabijay commented Apr 6, 2020 •

edited

Loading

shresthabijay commented Apr 6, 2020 •

edited

Loading

shresthabijay commented Apr 21, 2020 •

edited

Loading

Table view toggle with meta `=pinChildren`

Table view toggle with meta `=pinChildren`

shresthabijay commented May 11, 2020 •

edited

Loading

raineorshine commented May 11, 2020 •

edited

Loading