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: use elk edge label positioning #483

Merged
merged 8 commits into from
Dec 20, 2022

Conversation

gavin-ts
Copy link
Contributor

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

Summary

Elk layout positions edge labels on the side, but we position labels in the middle and this could lead to unexpected container padding. We now configure elk to position edge labels the same way so that it doesn't add this extra padding.

Details

e2e report changes with new test

_Users_gavinnishizawa_github_repos_d2_out_e2e_report html (4)

@alixander
Copy link
Collaborator

oh interesting, ELK naturally just placed the labels to the left?

@alixander
Copy link
Collaborator

can we tune it to... not always be left? https://www.eclipse.org/elk/reference.html

@gavin-ts
Copy link
Contributor Author

gavin-ts commented Dec 20, 2022

oh interesting, ELK naturally just placed the labels to the left?

default is their SMART_DOWN option, which usually places it on the left side. ref

can we tune it to... not always be left? https://www.eclipse.org/elk/reference.html

it doesn't look like we can put it on the edge itself, but I added some padding and switched it to the SMART_UP option which usually places labels on the right instead.

Screen Shot 2022-12-20 at 2 54 13 PM

@alixander
Copy link
Collaborator

they do allow it: https://www.eclipse.org/elk/reference/options/org-eclipse-elk-edgeLabels-inline.html

@gavin-ts gavin-ts force-pushed the use-elk-edge-label-positioning branch from 9594af0 to 6a064aa Compare December 20, 2022 23:12
@gavin-ts
Copy link
Contributor Author

they do allow it: https://www.eclipse.org/elk/reference/options/org-eclipse-elk-edgeLabels-inline.html

nice it looks more consistent with the other layouts in the center position

Screen Shot 2022-12-20 at 3 36 13 PM

@gavin-ts gavin-ts requested a review from alixander December 20, 2022 23:39
@gavin-ts gavin-ts marked this pull request as ready for review December 20, 2022 23:39
@gavin-ts gavin-ts enabled auto-merge December 20, 2022 23:48
@gavin-ts gavin-ts merged commit e69d476 into terrastruct:master Dec 20, 2022
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.

elk: investigate alignment issue
2 participants