Implement the double_circle shape including basic shape and the sketch.

[OneRain233]

Implement the double_circle shape including basic shape and the sketch.
#564

[ejulio-ts]

@OneRain233 can you include an image of your change? Quite hard to guess what it is from changes :)

Also, can you update this test case https://github.com/terrastruct/d2/blob/master/e2etests/stable_test.go#L26?

[OneRain233]

Here are the images that I have implemented :)

[OneRain233]

For the all_shape test, I found that the size of the double circle shape is larger than others. But when I ran the normal .2d file, it is normal. I dont know the reason.

x1:{shape:double_circle}
x2:{shape:circle}
x1 -> x2

[pleshevskiy]

@OneRain233 The shape stretches from the label size. I think you can make a little less internal padding

[OneRain233]

@OneRain233 The shape stretches from the label size. I think you can make a little less internal padding

Maybe I know the reasons, because the name double_circle is longer than others name.

[ejulio-ts]

cool!
Besides updating the test cases, you need to update their output too

TESTDATA_ACCEPT=1 ./ci/test.sh

and it'll update the SVGs and JSONs of the test cases, otherwise it'll break during the CI run

I'll let @alixander or @nhooyr make the final call on this one.

[alixander]

hi @OneRain233 thanks for taking the initiative here!

I don't think a separate shape is right for it though. There'd no 3d_square or multiple_circle or blue_oval; these are just attributes in D2. If this were to be implemented, it should be a style like double: true or double-border: true.

I don't personally know what this double-border style is for, but I've seen it used, and, recently in this: http://www.bpmb.de/images/BPMN2_0_Poster_EN.pdf . So I do think it's good to offer this in D2. Oh I guess I've seen it in state machines too

Btw, is there anything you felt was missing from the CONTRIBUTING.md? (did you use it at all? 😃 )

[OneRain233]

hi @OneRain233 thanks for taking the initiative here!

I don't think a separate shape is right for it though. There'd no 3d_square or multiple_circle or blue_oval; these are just attributes in D2. If this were to be implemented, it should be a style like double: true or double-border: true.

I don't personally know what this double-border style is for, but I've seen it used, and, recently in this: http://www.bpmb.de/images/BPMN2_0_Poster_EN.pdf . So I do think it's good to offer this in D2. Oh I guess I've seen it in state machines too

Btw, is there anything you felt was missing from the CONTRIBUTING.md? (did you use it at all? smiley )

Yes, I just want to use the double circle in the accept state of state machine.
I think a style attribute is better than a new shape.

[alixander]

@OneRain233 how about double-border: true | false? not double, to avoid confusion with multiple

[OneRain233]

@OneRain233 how about double-border: true | false? not double, to avoid confusion with multiple
Yes, double-border: true | false is better.

[OneRain233]

Should the tag double-border be used only for circles or for all shapes? It may be strange for some shapes to use this tag? @alixander

[OneRain233]

I have made double-border for the circle shape only.

[alixander]

@OneRain233 I think for circles, ovals, squares, and rectangles. you can see how 3d is restricted

[OneRain233]

Now, it looks like this.

[alixander]

sorry for the delay in reviewing this. it looks like all the tests changed because you ran on go 1.19. Can you run with go 1.18 please?

The merge conflicts look like a lot but should mostly be just regenerating tests

[alixander]

this turned out to look really nice, especially in sketch mode! Just a few more things:

Looks like you also need to account for when pad is 0.
d2 --pad 0 input.d2 gave me this:

You can see here for example of fixing: #685

Also just more merge conflicts with tests and changelog, sorry about that.

[ejulio-ts]

@OneRain233, yes.
You merge/rebase the current master and update the tests.

[alixander]

@OneRain233 looks like all the commits were pulled in

[OneRain233]

I think the issue #685 is solved. Now it looks like this.

[alixander]

ready to merge after 1 minor thing.

[alixander]

looks like one more merge + test regeneration plz @OneRain233

[alixander]

we'll just merge and fix the conflict in another commit on master. Thank you @OneRain233 ! Sorry for the delays in this one

[nhooyr]

@alixander note for the future, github normally automatically gives you write access to a contributor's branch unless the contributor has declined your access.