cybersemics / em

A beautiful, minimalistic note-taking app for personal sensemaking.
Other
258 stars 88 forks source link

Table: Heights are incorrect on initial render #1889

Closed raineorshine closed 21 hours ago

raineorshine commented 1 month ago

Steps to Reproduce

- a
  - =view
    - Table
  - =pin
    - true
  - Boston
    - c
  - Chicago
    - d
- x

Import the above thoughts and refresh the page.

Current Behavior

When the page is refreshed, the heights are wrong. When the cursor is set on a, the heights are corrected.

https://github.com/cybersemics/em/assets/750276/3ffd13f0-2dae-41ae-a693-932fc3c8646d

Expected Behavior

Heights should be recalculated and positions updated after thoughts are initially rendered.

emonidi commented 2 weeks ago

Hi There, This architecture will only bring you more and more headaches down the line. You need to build a real dom , a nested dom structure and not calculating the height of each element dynamically. I am sorry if I am coming too hard but in 8 years with react I haven't seen such thing. I spent an hour going around the code and again if you do not fix the way you are rendering the tree and use real dom elements and style of display flex this thing will not work. Anyway I managed to solve this bug. Here's a screencast: https://www.loom.com/share/732e2369ae694f7cbfc5b1df48030508?sid=0815b244-c8d0-4a74-88aa-a812639703ae I am sorry if I am coming too hard, that is not my intetion

raineorshine commented 2 weeks ago

Hi, thanks for your input. The reason for the architecture is that it enables animation between any two states of the tree. For example, if an item is deleted out of the middle of the tree and its descendants are shifted up a level, the descendants can be smoothly animated to their new locations. I'm not sure how that would work if you relied on the normal DOM layout.

emonidi commented 2 weeks ago

Hi, thanks for your input. The reason for the architecture is that it enables animation between any two states of the tree. For example, if an item is deleted out of the middle of the tree and its descendants are shifted up a level, the descendants can be smoothly animated to their new locations. I'm not sure how that would work if you relied on the normal DOM layout.

The first thing that comes to mind is once you delete the text from a node you would get its calculated height, set (enforce) its height via css and then animate down to zero via js or css transition. When animation is done you remove the node completely from the DOM. That would ensure smooth animation of the nodes below appearing to slide up. Same strategy could be applied both horizontally and vertically.

raineorshine commented 2 weeks ago

That may work for a simple animation, but I don't think it would be as easy for a more complex animation that involved many items changing locations. For example, converting a table to a nested list and having all items animate into the correct place.

emonidi commented 2 weeks ago

Have you got a test case that I can reproduce and see what exactly do you mean?

raineorshine commented 2 weeks ago

Sure. Take a slightly different test case:

- a
  - =children
    - =pin
      - true
  - Boston
    - c1
    - c2
  - Chicago
    - d1
    - d2

If table view is activated on thought 'a' (alt/option + shift + T or click image in the toolbar), the grandchildren are smoothly animated into their positions in the second column, which involves changes in both x and y coordinates. (The first column is not animated currently due to a bug.)

https://github.com/cybersemics/em/assets/750276/86c02027-64cb-4d80-a5e5-a28ef4485174

Other commands, such as Collapse Context (command/ctrl + alt/option + c) or Subcategorize One (command/ctrl + alt/option + o) involve animating thoughts across levels as well. And the tree may change for other reasons: activating undo/redo, loading thoughts from storage, or activating a command before the animation for the last command has completed. Using a custom layout algorithm based on absolute x and y coordinates enables the animation between any two states without ad hoc solutions for different cases.

emonidi commented 2 weeks ago

Although it seems hard it is still possible to create animations of flex css, which will definatelly streamline the development and make it less error prone. Take a look at the following demo. I understand that the project is already in an advanced phase but if you consider at some point a v2 consider this example: https://codepen.io/osublake/pen/dMLQJr

raineorshine commented 2 weeks ago

Thanks. That does seem quite flexible. It would be a large undertaking no doubt, but I'll keep it in mind if I hit a wall with my current approach. It would definitely have some performance benefits without having to measure the DOM all the time.