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

always allow width/height on grid containers #1731

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Nov 14, 2023

Summary

Allow setting width/height on grid containers even when the layout plugin doesn't have that feature.

Details

  • fixes dagre: dimensions on grid containers #1722
  • adds grid_container_dimensions test
  • before e2e_test.go:203: unexpected error: Object "grid" has attribute "width" and/or "height" set, but layout engine "dagre" does not support dimensions set on containers. See https://d2lang.com/tour/layouts/#layout-specific-functionality for more.
  • updates grid layout to size according to width/height and center contents accordingly

grid container with width: 200; height: 200

Screenshot 2023-11-14 at 12 10 26 PM

@gavin-ts gavin-ts marked this pull request as ready for review November 14, 2023 21:47
@gavin-ts gavin-ts merged commit e5b3bc2 into terrastruct:master Nov 15, 2023
3 checks passed
@alixander
Copy link
Collaborator

Screen Shot 2023-11-15 at 1 24 00 AM

wait, why doesn't the grid cells expand? this feature should not exist as is, my mistake to approve without checking. this is completely wrong/unexpected behavior, as if it were creating a new container

@gavin-ts
Copy link
Contributor Author

Screen Shot 2023-11-15 at 1 24 00 AM wait, why doesn't the grid cells expand? this feature should not exist as is, my mistake to approve without checking. this is completely wrong/unexpected behavior, as if it were creating a new container

oh interesting, didn't realize that was what #1722 wanted

@gavin-ts
Copy link
Contributor Author

so to confirm, the idea is any extra space (not including gap) set on the grid container will be divided among the cells, expanding the cells too?

@gavin-ts
Copy link
Contributor Author

gavin-ts commented Nov 15, 2023

this is completely wrong/unexpected behavior, as if it were creating a new container

hmm but it is a container, it is just hidden when there is grid-gap: 0 and an outside label and rectangular non-transparent cells.

@gavin-ts
Copy link
Contributor Author

hmm, I think scaling the cells should be done on the cells itself, and the grid container's size is independent

E.g. If you have a hexagon grid container, and want to increase the height, I would expect this change from left hexagon to right, and I wouldn't expect it to stretch the cells.

Screenshot 2023-11-15 at 3 08 10 PM

@alixander
Copy link
Collaborator

I see what you're saying about it being a container. That's right, my bad.

Anyway, so the decision is just whether to stretch the grid cells.

I think the motivation for me is just I can't imagine a case where one would want to adjust sizing without stretching grid cells. Like you only would want to specify grid dimensions if your intention is to control how big or small the cells look in the diagram.

@gavin-ts
Copy link
Contributor Author

gavin-ts commented Nov 16, 2023

a case where one would want to adjust sizing without stretching grid cells.

One example: Let's say you have this diagram and want to make the hexagon the same height as the circle

Screenshot 2023-11-16 at 11 45 03 AM

I think the result should be:

Screenshot 2023-11-16 at 11 45 23 AM

Rather than:

Screenshot 2023-11-16 at 11 56 12 AM

Unless you specify that explicitly.

Alternatively, this is the starting state (which also seems unexpected to me):

Screenshot 2023-11-16 at 11 52 27 AM

but then they are (mostly) even when you size the hexagon:

Screenshot 2023-11-16 at 11 46 48 AM

What behavior do you think this example should have?

vars: {
  grid-container: {
    label: ""
    grid-gap: 10
    grid-rows: 1
    grid-columns: 4
    a
    b
    c
    d
  }
}

circle: {
  shape: circle
  ...${grid-container}
  width: 395
}

hexagon: {
  shape: hexagon
  ...${grid-container}
  # width: 395
  # height: 395
}

direction: right
circle -> hexagon

Update: this also means different shapes would stretch the same cells different amounts at same dimensions

Screenshot 2023-11-16 at 12 16 45 PM

vs

Screenshot 2023-11-16 at 12 17 10 PM

maybe this behavior can be a setting e.g. grid-fill: true, fill-grid: true, or stretch-grid: true?

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.

dagre: dimensions on grid containers
2 participants