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

layout with nested grids #1309

Merged
merged 8 commits into from
May 11, 2023
Merged

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented May 9, 2023

Summary

Updating layout to support nested layouts within grids.

Details

  • allow containers for grid cells
  • allow grids within grid cells
  • adds grid_nested test

new test

Screen Shot 2023-05-08 at 6 47 17 PM

@gavin-ts gavin-ts force-pushed the nested-layout branch 2 times, most recently from fcf4fb9 to 99b8c67 Compare May 10, 2023 01:22
@gavin-ts gavin-ts requested a review from a team May 10, 2023 01:22
@gavin-ts gavin-ts marked this pull request as ready for review May 10, 2023 01:22
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.

  1. i feel like grid-gap should apply to the padding? i'm thinking of this guy's use case: markdown: Tables #160 (comment) . or maybe nested grid just has 0 padding. (though nested container still does)

Screen Shot 2023-05-10 at 2 17 51 PM

  1. do we still have to prevent edges when a grid cell is a container for a non-grid?

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.

  1. i feel like grid-gap should apply to the padding? i'm thinking of this guy's use case: markdown: Tables #160 (comment) . or maybe nested grid just has 0 padding. (though nested container still does)

Screen Shot 2023-05-10 at 2 17 51 PM

  1. do we still have to prevent edges when a grid cell is a container for a non-grid?

@gavin-ts
Copy link
Contributor Author

  1. i feel like grid-gap should apply to the padding? i'm thinking of this guy's use case: markdown: Tables #160 (comment) . or maybe nested grid just has 0 padding. (though nested container still does)

I was thinking we could add container padding customization, but in its own PR. We will probably have to make labels have priority though, otherwise there's no place for them in nested containers with padding 0.

The linked use case has labels and padding around each table. https://github.com/jfudickar/json2puml/blob/main/documentation/images/rocket/rocket_5e9d0d95eda69973a809d1ec.default.svg

Screen Shot 2023-05-10 at 2 17 51 PM

Is there a bug here with the order? is last supposed to be at the end? what's the d2?

  1. do we still have to prevent edges when a grid cell is a container for a non-grid?

currently yes, but I want to follow up with a refactoring of all of our different layout types (constant nears, sequence diagram, grid, + default) with this refactor, edges within a container in a grid cell would be fine as would a sequence diagram in a grid cell.

@alixander
Copy link
Collaborator

alixander commented May 10, 2023

no bug. was just demonstrating the padding

The Universe: {
  grid-rows: 3

  FirstTwo.width: 300
  Last.style.fill: red

  TALA: "" {
    grid-rows: 3
    grid-gap: 0
    TALA
    D2
    Cloud
  }
  D2
  Cloud
  Terrastruct.width: 400
}

@gavin-ts
Copy link
Contributor Author

gavin-ts commented May 10, 2023

no bug. was just demonstrating the padding

The Universe: {
  grid-rows: 3
  grid-gap: 0
 ...

ok you can add this^, but yes container padding still exists

Screen Shot 2023-05-10 at 2 47 44 PM

@gavin-ts
Copy link
Contributor Author

okay updated grid layout to also use gap values for container padding and added a test:

Screen Shot 2023-05-10 at 3 26 11 PM

e2e delta

_Users_gavinnishizawa_github_repos_d2_e2etests_out_e2e_report html

@gavin-ts gavin-ts requested a review from alixander May 10, 2023 23:46
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.

only lightly reviewed

ci/release/changelogs/next.md Show resolved Hide resolved
d2layouts/d2grid/grid_diagram.go Outdated Show resolved Hide resolved
@gavin-ts gavin-ts enabled auto-merge May 11, 2023 00:26
@gavin-ts gavin-ts merged commit 9664fd6 into terrastruct:master May 11, 2023
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