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

Elk border radius #913

Merged
merged 6 commits into from
Mar 1, 2023
Merged

Conversation

ShupingHe
Copy link
Contributor

@ShupingHe ShupingHe commented Feb 27, 2023

#673

add support for customized border-radius of elk connection, the following script:

a -> b
a -> c: {
  style: {
    border-radius: 0
  }
}
a -> e: {
  style: {
    border-radius: 5
  }
}
a -> f: {
  style: {
    border-radius: 10
  }
}
a -> g: {
  style: {
    border-radius: 20
  }
}

will produce:
image

you can get this report by run:
./ci/e2ereport.sh -test-case=elk_border_radius

@ShupingHe
Copy link
Contributor Author

ShupingHe commented Feb 27, 2023

@alixander raised a pull request, pls help to take a look, and i will complete the related docs once code review is done

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.

@donglixiaoche great! please add some e2e tests and screenshots. see #895 for an ongoing pull request example

@alixander
Copy link
Collaborator

@ShupingHe ShupingHe force-pushed the elk-border-radius branch 2 times, most recently from 85d8e75 to c23542e Compare February 28, 2023 04:06
@ShupingHe
Copy link
Contributor Author

ShupingHe commented Feb 28, 2023

@alixander i got one question to discuss with you

well, previously we got a fixed 10 border-radius for every elk connection, i think we should keep this as default behavior right? if so, then i think there are two choices:

  1. inside the pathData function, when draw an elk connection, if the border-radius equals 0, then set to 10, but this will prevent user from set border-radius to 0 too

image

so i add a default -1 if no border-radius is set, but this will result in a lot of fails of our test cases
image

  1. add a default BorderRadius: 10 for BaseConnection, but this will cause the same issue above

image

do you have any good ideas?

@alixander
Copy link
Collaborator

@donglixiaoche ohh i see.

add a default BorderRadius: 10 for BaseConnection

i think we should do this. that's the most accurate anyway. all the tests will need to be regenerated (TESTDATA_ACCEPT=1 ./ci/test.sh), but that's okay.

@ShupingHe
Copy link
Contributor Author

@alixander already signed my commits, added an e2e testcase and regenerated testcases

d2target/d2target.go Show resolved Hide resolved
e2etests/stable_test.go Outdated Show resolved Hide resolved
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.

tested locally. works well. 💯

@alixander alixander merged commit 92bb25a into terrastruct:master Mar 1, 2023
@alixander
Copy link
Collaborator

oops forgot to ask you to update changelog. no big deal, i just made a commit to do it: 91e0bbd . thanks @donglixiaoche !

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.

2 participants