Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new grid layout with rows/columns #1122

Merged
merged 31 commits into from
Apr 7, 2023
Merged

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Apr 1, 2023

Summary

Adds a new "grid diagram" layout to place nodes in rows and/or columns.
Use keyword grid-rows: N or grid-columns: N on a container to layout the container's children accordingly.
Edges within grids, and containers in grids are not currently supported.

Details

grids can be created by settings rows or columns

Screen Shot 2023-04-04 at 8 39 03 PM

layout will attempt to evenly place nodes into the number of rows or columns

Screen Shot 2023-04-04 at 8 39 57 PM

with both rows and columns, each row's nodes will be the same height and each column's node the same width

Screen Shot 2023-04-05 at 11 34 49 AM

ordering: if rows is set first, nodes will be placed along rows

Screen Shot 2023-04-04 at 8 41 35 PM

if you add nodes exceeding the capacity, it is ok, it will just continue the same way

Screen Shot 2023-04-04 at 8 43 55 PM

use rows: 3 + node width/height to build desired layout (new executive_grid test)

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_stable_executive_grid_dagre_sketch exp svg (1)

new teleport_grid test

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_stable_teleport_grid_dagre_sketch exp svg (6)

new dagger_grid test

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_stable_dagger_grid_dagre_sketch exp svg

@gavin-ts gavin-ts requested a review from a team April 1, 2023 02:17
@gavin-ts gavin-ts marked this pull request as ready for review April 1, 2023 02:17
Copy link
Contributor

@ejulio-ts ejulio-ts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the new d2grid/layout.go file. This is something I try to do, but it may not be possible in some cases.

The first thing in the file is newGrid, followed by Layout, layoutGrid and withoutGrids

But the order of the calls in Layout, withoutGrids, layoutGrid and newGrid. So, when reviewing/reading I had to keep going back and forth to find things. In general, I try to keep them in sequence as they are called. In some cases it makes it easier to follow/understand.

Another one is that, I think we should treat this as we do with clusters in TALA. Within a grid, nodes should have a standard size because of the symmetry it adds to the end result.

Should this handle nested grids? if so, can you add a test case?

Finally, I don't think I saw that no edges to descendants is enforced. If so, I think this is important to raise as an error in the compiler

d2compiler/compile.go Outdated Show resolved Hide resolved
d2layouts/d2grid/layout.go Outdated Show resolved Hide resolved
d2layouts/d2grid/layout.go Outdated Show resolved Hide resolved
d2layouts/d2grid/layout.go Show resolved Hide resolved
d2layouts/d2grid/layout.go Show resolved Hide resolved
d2layouts/d2grid/layout.go Outdated Show resolved Hide resolved
@alixander
Copy link
Collaborator

alixander commented Apr 4, 2023

TODO add compiler validation for nodes in grid container must not have edges or descendants

@donglixiaoche wrote something similar here for edges: #1071

perhaps you could reuse that

@alixander
Copy link
Collaborator

alixander commented Apr 5, 2023

just to test its flexibility, can it produce this? (the circled part)

Screen Shot 2023-04-04 at 7 34 59 PM

@gavin-ts
Copy link
Contributor Author

gavin-ts commented Apr 5, 2023

just to test its flexibility, can it produce this? (the circled part)

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_stable_executive_grid_dagre_sketch exp svg (1)

Here's what it looks like currently. Don't have recursive grids yet, but it should be doable since we don't allow edges within grids.

@gavin-ts gavin-ts requested a review from ejulio-ts April 5, 2023 04:06
@alixander
Copy link
Collaborator

alixander commented Apr 5, 2023

let's call these "Grid diagrams" https://discord.com/channels/1039184639652265985/1039184640285610147/1093008211600228484

can you make the appropriate namings in the code?

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2023-04-05 at 11 41 17 AM

The Universe: {
  columns: 3

  Planets
  Solar Systems
  Galaxies

  Venus
}

@gavin-ts
Copy link
Contributor Author

gavin-ts commented Apr 5, 2023

The Universe: {
  columns: 3
...

I'm fixing this now, it is the TODO in the pr

Update: Fixed, now the 4 nodes are arranged over 3 columns as specified

Screen Shot 2023-04-05 at 8 12 47 PM

@gavin-ts gavin-ts requested a review from alixander April 6, 2023 03:13
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Universe: {
  columns: 3
  rows: 2

  Planets
  Solar Systems
  Galaxies

  Venus
}

Screen Shot 2023-04-05 at 8 17 26 PM

@gavin-ts
Copy link
Contributor Author

gavin-ts commented Apr 6, 2023

With columns specified first here, it places the available nodes along each column first, but there aren't enough nodes. With more nodes it will fill the 3 columns:

Screen Shot 2023-04-05 at 8 47 07 PM

Screen Shot 2023-04-05 at 8 48 20 PM

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's name the keywords grid-columns and grid-rows to avoid parking another two common keywords, which especially could come up in sql tables.

d2compiler/compile.go Outdated Show resolved Hide resolved
d2compiler/compile.go Outdated Show resolved Hide resolved
d2graph/grid_diagram.go Outdated Show resolved Hide resolved
d2layouts/d2grid/grid_diagram.go Outdated Show resolved Hide resolved
d2layouts/d2grid/grid_diagram.go Outdated Show resolved Hide resolved
d2layouts/d2grid/grid_diagram.go Outdated Show resolved Hide resolved
d2layouts/d2grid/grid_diagram.go Outdated Show resolved Hide resolved
d2layouts/d2grid/grid_diagram.go Outdated Show resolved Hide resolved
@gavin-ts gavin-ts requested a review from alixander April 6, 2023 22:49
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't review algorithms too much, tested and played around with it and works great.

d2layouts/d2grid/layout.go Show resolved Hide resolved
d2layouts/d2grid/layout.go Show resolved Hide resolved
@gavin-ts gavin-ts merged commit b3511d1 into terrastruct:master Apr 7, 2023
@shykes
Copy link

shykes commented Apr 9, 2023

Thanks for the Dagger shoutout 😁❤️

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.

specify rows/cols
4 participants