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

render: support width/height on shapes #498

Merged
merged 11 commits into from
Dec 29, 2022

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Dec 22, 2022

Summary

Support setting width/height on non-image shapes and handles classes/tables with an empty header.

Details

  • fixes layout: Make width and height keywords work for non-image shapes #497
  • added e2e test with width/height
  • width/height can now be set on shapes other than images
  • normally shapes are sized at label dimensions + 100 padding, but if the set dimensions are smaller it will only go down to label dimensions + 0 padding as a minimum size
  • similarly for classes, sql tables, text and code, as they are rendered at their minimum size already they will only be able to grow in size (see code examples in test below)
  • for containers, width and height end up ignored by dagre/elk unfortunately, so this will be a compiler error
  • fixes an edge case where an unnamed class would always be sized at 100x100 and added a regression test
  • compiler will error if width and height are different on a circle or square shape

e2e test

_Users_gavinnishizawa_github_repos_d2_out_e2e_report html

unnamed class regression test

_Users_gavinnishizawa_github_repos_d2_out_e2e_report html (1)

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 main thing here is handling containers. i think width and height only apply to non-containers for now?

@gavin-ts gavin-ts force-pushed the set-width-height-on-shapes branch 7 times, most recently from 8d8c826 to 397e14c Compare December 28, 2022 23:02
@gavin-ts gavin-ts requested a review from a team December 28, 2022 23:04
@gavin-ts gavin-ts marked this pull request as ready for review December 28, 2022 23:04
@gavin-ts gavin-ts enabled auto-merge December 28, 2022 23:06
@gavin-ts gavin-ts requested a review from alixander December 28, 2022 23:06
@ejulio-ts
Copy link
Contributor

ejulio-ts commented Dec 29, 2022

the main thing here is handling containers. i think width and height only apply to non-containers for now?

makes sense as TALA doesn't take it under consideration for proper placement.
In this case, should we add an error/warning to the compiler?
Similarly, if the set width/height is smaller than the label requirement, should we warn the user as they might get a wrong idea of the sizing if based on a relative size that we didn't as they expected

@gavin-ts
Copy link
Contributor Author

the main thing here is handling containers. i think width and height only apply to non-containers for now?

makes sense as TALA doesn't take it under consideration for proper placement. In this case, should we add an error/warning to the compiler? Similarly, if the set width/height is smaller than the label requirement, should we warn the user as they might get a wrong idea of the sizing if based on a relative size that we didn't as they expected

I think we could add a compiler warning for setting width or height on a container, but do we have currently have non-error warnings?

I don't think setting the width or height to a value smaller than the minimum size should be a compiler warning, I think should just be the described behavior in the documentation.

@alixander
Copy link
Collaborator

alixander commented Dec 29, 2022

I think we could add a compiler warning for setting width or height on a container, but do we have currently have non-error warnings?

sounds nice but in practice it'd be annoying, because the container changes depending on layout. so from one compile to the next, the warning appears unpredictably

@gavin-ts
Copy link
Contributor Author

I think we could add a compiler warning for setting width or height on a container, but do we have currently have non-error warnings?

sounds nice but in practice it'd be annoying, because the container changes depending on layout. so from one compile to the next, the warning appears unpredictably

The container structure should be the same regardless of layout, I think @ejulio-ts means always warning if you define width on a container shape, regardless of the value.

@ejulio-ts
Copy link
Contributor

yeah, for containers once you set, you get the error/warning.
for other shapes, if there's a size mismatch, the same (basically, for containers we assume it is always a mismatch)

@gavin-ts gavin-ts requested a review from alixander December 29, 2022 04:56
@gavin-ts gavin-ts force-pushed the set-width-height-on-shapes branch from abe02fc to ae4e636 Compare December 29, 2022 05:29
@gavin-ts gavin-ts enabled auto-merge December 29, 2022 05:30
@gavin-ts gavin-ts merged commit 07500f4 into terrastruct:master Dec 29, 2022
@@ -2,9 +2,11 @@

- Tooltips can be set on shapes. See [https://d2lang.com/tour/tooltips](https://d2lang.com/tour/interactive). [#548](https://github.com/terrastruct/d2/pull/548)
- Links can be set on shapes. See [https://d2lang.com/tour/tooltips](https://d2lang.com/tour/interactive). [#548](https://github.com/terrastruct/d2/pull/548)
- The `width` and `height` attributes are no longer restricted to images and can be applied to non-container shapes. [#498](https://github.com/terrastruct/d2/pull/498)
Copy link
Contributor

Choose a reason for hiding this comment

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

non-container shapes

Maybe, non-container objects as there're no such thing as container shapes

Copy link
Collaborator

Choose a reason for hiding this comment

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

i wanted to call every "node" a shape, as object is pretty all-encompassing. but i'm not 100% sure about that route

Copy link
Contributor

Choose a reason for hiding this comment

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

well, from a changeling perspective, shape reminds me of shape keyword and not the objects themselves

Copy link
Contributor Author

@gavin-ts gavin-ts Dec 29, 2022

Choose a reason for hiding this comment

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

Screen Shot 2022-12-29 at 12 07 15 PM

from https://d2lang.com/tour/containers :

server
# Declares a shape inside of another shape
server.process

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.

layout: Make width and height keywords work for non-image shapes
3 participants