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

dagre: stop container edges at container label #880

Merged

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Feb 23, 2023

Summary

when an edge goes to a container, it often conflicts with the container's label so we want to stop the edge at its label instead.

Details

  • stop dagre edge to container at the container's label if it runs into it
  • add todo e2e test
  • Note: this doesn't affect edges to container children which may also cross the container label

e2ereport changes

_Users_gavinnishizawa_github_repos_d2_e2etests_out_e2e_report html (1)

new e2e test before/after

Screen Shot 2023-02-23 at 11 38 55 AM

closes #823

@gavin-ts gavin-ts force-pushed the adjust-dagre-edge-for-container-label branch from a65fddf to 3584bf0 Compare February 23, 2023 19:46
@gavin-ts gavin-ts requested a review from alixander February 23, 2023 19:47
@gavin-ts gavin-ts marked this pull request as ready for review February 23, 2023 19:47
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.

should we add label.PADDING on all sides to it too? to cover these cases

Screen Shot 2023-02-23 at 12 36 31 PM

@gavin-ts
Copy link
Contributor Author

should we add label.PADDING on all sides to it too? to cover these cases

Screen Shot 2023-02-23 at 12 36 31 PM

good idea updated:

_Users_gavinnishizawa_github_repos_d2_e2etests_out_e2e_report html (3)

@gavin-ts gavin-ts requested a review from alixander February 23, 2023 21:04
@alixander
Copy link
Collaborator

can you add @bo-ku-ra 's example as an e2e test? #823 (comment)

@gavin-ts
Copy link
Contributor Author

can you add @bo-ku-ra 's example as an e2e test? #823 (comment)

Screen Shot 2023-02-23 at 2 44 14 PM

@alixander
Copy link
Collaborator

oh interesting. so arrowhead label and shape can overlap. that's a different issue then, and layout engine agnostic. i'll change that issue to reflect

@gavin-ts gavin-ts merged commit 1a27dee into terrastruct:master Feb 23, 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.

arrowhead label and shape may clash
2 participants