Skip to content

Conversation

@Rounak-stha
Copy link
Contributor

This PR adds the feature of allowing different tree orientations mentioned in #24 .
One thing to note is that since the while svg is rotated, so for Left - Right or Right -> Left orientations the width and height are flipped.

Results:

L-R:
image

R-L:
image

D-T:
image

@castarco
Copy link
Contributor

castarco commented Oct 1, 2023

Hi @Rounak-stha 😄 , thank you for the PR.

There are a some details that I'd like to verify, though.

  1. Because it's relying on rotation (instead of changing how layout coordinates are translated to the canvas), I'm not sure how it will work in non-square canvas (basically when the canvas rectangle is quite stretched). We might need some extra "stories" to check its visual appearance.
  2. Going back to rotations, we might also need to check how this affects text display inside the nodes. Is it rotated as well, or does it preserve its left to right orientation?
  3. While top->down -> left->right can be easily down with rotation, I'm not sure it's the right choice for bottom->up and right->left should be done that same way (I think mirroring makes more sense, since it preserves the order among siblings)

@Rounak-stha
Copy link
Contributor Author

Hey @castarco
Let me go through your concerns.

  1. Since the whole SVG is rotated, the dimension of the SVG box does not impose any issue. Initially, I chose to rotate individual children of svg and was getting the issue you are concerned about. There's one thin to note though, when Left-Right | Right-Left orientation is chosen, the dimension is flipped (rotated).

  2. I completely missed the orientation of the text, Thanks for mentioning. I've fixed it in the latest commit.

  3. I am confused with this point. Won't mirroring actually flip the order of the children? I thought I was onto something and did some changes but the output's not changed (might need to revert that).

Here are the results:

Normal Orientation:
image

Left-Right:
image

Right-Left:
image

Down_Top:
image

@castarco
Copy link
Contributor

castarco commented Oct 1, 2023

Hi again @Rounak-stha . I checked, and what happens when we rotate the tree is not OK (it's not the same to rotate the content as to rotate the canvas), as it does not respect the width and height that we set for the canvas (it switches them).

Screenshot 2023-10-01 at 14 45 35

@Rounak-stha
Copy link
Contributor Author

I am working on fixing this
Instead of rotating the canvas, I'll be swapping the coordinates of the children. It will preserve the dimension of the viewbox.

I am still confused on the issue of ordering of child nodes.

@Rounak-stha
Copy link
Contributor Author

Rounak-stha commented Oct 2, 2023

Hi @castarco

I updated the orientation logic and I think it works well now
but before pushing the changes, I wanted to confirm what you meant here

3. While `top->down` -> `left->right` can be easily down with rotation, I'm not sure it's the right choice for `bottom->up` and `right->left` should be done that same way (I think mirroring makes more sense, since it preserves the order among siblings)

The way I am thinking is that the layout that the rotation logic rendered was correct in a way that the left and right child would retain their position (example in above comments) but when mirrored the positions would get swapped.

Here's what the trees look like after mirroring:
image

- Respect the Height and Width on Different Orientation
@castarco castarco merged commit 898142c into Coder-Spirit:main Oct 14, 2023
@castarco
Copy link
Contributor

@Rounak-stha merged 👍🏻 🚀

@Rounak-stha
Copy link
Contributor Author

Great
Looking forward to contributing more to the project. ⭐🎊🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants