-
Notifications
You must be signed in to change notification settings - Fork 466
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
d2target: add support for cross arrowhead #2190
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 PR!
Do you mind adding a line to next.md
?
I know you emulated what MermaidJS does with the cross, as depicted in the issue, but I wonder if that's ideal. Maybe Mermaid's implementation is not ideal, and a cross really should look like an x
instead of this snowflake-look (*
) with the endpoint extending all the way. I personally haven't used this endpoint before, so I don't know the answer. What do you think? Also cc @bo-ku-ra who originally filed the issue.
excellent! @MxHonesty @alixander the japanese are unique. most Japanese read the following. e.g. source-arrowhead: OK {
shape: circle
style.filled: false
}
target-arrowhead: NG {
shape: cross
} what is better for non-Japanese? |
@bo-ku-ra Good insight, How did you get that screenshot from Mermaid? What's the code for it? |
|
Hah, interesting. Well in any case, I think D2's E.g. the sketch version looks right but the non-sketch version looks like a snowflake @MxHonesty could you investigate that in this PR? I think the |
Sure, i will try doing it this way! |
hi @MxHonesty just wanted to check if you're still interested in pursuing this. No obligation of course, I can carry on what you have already if not. |
hi @alixander sorry for the inactivity. i had some busy weeks but was actually planning on picking up this work again this weekend if that's okay with you |
@MxHonesty No worries at all |
Add support for cross arrowhead in d2target (and d2sketch).
closes #2028
Why i chose polygon
In order to keep the look of the suggestion i went with a rotated polygon instead of simple crossing lines. Would appreciate feedback on this.
End-to-End Tests:
CrossArrowhead
ine2etests/stable_test.go
.CrossArrowhead
ine2etests/testdata/files/cross_arrowhead.d2
.CrossArrowhead
tests ine2etests/testdata/txtar/sketch-cross-arrowhead/dagre/board.exp.json
ande2etests/testdata/txtar/sketch-cross-arrowhead/elk/board.exp.json
.CrossArrowhead
in thee2etests/txtar.txt
file.Images