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

Update arrowhead label positioning and use label dimensions #1207

Merged
merged 14 commits into from
Apr 17, 2023

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Apr 15, 2023

Summary

Improves arrowhead positioning and handling of large arrowhead labels.

Details

  • fixes render: measure arrowhead label text #183
  • usually arrowhead labels are 1 or *, but the scenario of having longer arrowhead labels wasn't accounted for
  • instead of assuming a 1 character size for arrowhead labels, measure the text and use that for placement
  • update renderer's bounding box for arrowhead labels
  • updates arrowhead positioning according to arrowhead size
  • adds long_arrowhead_label test
  • adds arrowhead_sizes_with_labels test
  • Note: this is different than wip: Consider arrowhead for label position #1192 which is adjusting edge label position around arrowheads, but this is for arrowhead labels
  • TODO use edge label dimensions to adjust dagre/elk spacing adjust edge spacing/minimum length #1214

before

Screen Shot 2023-04-17 at 1 18 06 PM

after

Screen Shot 2023-04-17 at 1 18 22 PM

long arrowhead before/after

_Users_gavinnishizawa_github_repos_d2_e2etests_out_e2e_report html (7)

new test before

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_regression_arrowhead_sizes_with_labels_dagre_sketch exp svg

new test after

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_regression_arrowhead_sizes_with_labels_dagre_sketch got svg

@gavin-ts gavin-ts force-pushed the arrowhead-label-dimensions branch 2 times, most recently from 46ab88a to 974daad Compare April 17, 2023 18:17
@gavin-ts gavin-ts changed the title Arrowhead label dimensions Update arrowhead label positioning and use label dimensions Apr 17, 2023
@gavin-ts gavin-ts marked this pull request as ready for review April 17, 2023 20:25
@gavin-ts gavin-ts requested a review from a team April 17, 2023 20:25
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.

very nice. just some nits. can you include a sketch test as well to make sure it works in sketch mode?

lib/label/label.go Show resolved Hide resolved
e2etests/e2e_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ejulio-ts ejulio-ts left a comment

Choose a reason for hiding this comment

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

why are there so many cases updated? Specially, cases without arrowhead labels

d2graph/d2graph.go Outdated Show resolved Hide resolved
e2etests/e2e_test.go Outdated Show resolved Hide resolved
@gavin-ts
Copy link
Contributor Author

very nice. just some nits. can you include a sketch test as well to make sure it works in sketch mode?

added as a sketch test
_Users_gavinnishizawa_github_repos_d2_d2renderers_d2sketch_testdata_long_arrowhead_label_sketch exp svg

@gavin-ts
Copy link
Contributor Author

why are there so many cases updated? Specially, cases without arrowhead labels

updated d2target srcLabel and dstLabel to Text type

@@ -1227,10 +1221,8 @@
       "id": "(h.m.s -> a.f.g)[0]",
       "src": "h.m.s",
       "srcArrow": "none",
-      "srcLabel": "",
       "dst": "a.f.g",
       "dstArrow": "triangle",
-      "dstLabel": "",
       "opacity": 1,
       "strokeDash": 0,
       "strokeWidth": 2,

https://github.com/terrastruct/d2/pull/1207/files?file-filters%5B%5D=.go&file-filters%5B%5D=.pdf&show-viewed-files=true#diff-b014022cd772b25135d09ba644217b8da2b537c7b7a5d670a893d173a289d18cR455

@gavin-ts gavin-ts requested review from ejulio-ts and removed request for ejulio-ts April 17, 2023 22:05
@gavin-ts gavin-ts enabled auto-merge April 17, 2023 22:22
@gavin-ts gavin-ts merged commit eae415a into terrastruct:master Apr 17, 2023
2 checks passed
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.

render: measure arrowhead label text
3 participants