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

Class and sqltable border radius #982

Merged

Conversation

ShupingHe
Copy link
Contributor

@ShupingHe ShupingHe commented Mar 6, 2023

closes #689
this pr add support for apply border-radius to class and sql_table

the image below can be generate by run ./ci/e2ereport.sh -test-case=class_and_sqlTable_border_radius:
image

@ShupingHe
Copy link
Contributor Author

ShupingHe commented Mar 6, 2023

@alixander after fetch the remote repo, seems like there are something wrong with my local env, do you have any ideas how to resolve it? i already asked in discord help channel.

unexpected error: failed to wait xmain test: e2etests-cli/d2: fdopendir /usr/bin/gem: not a directory

@alixander
Copy link
Collaborator

hey @donglixiaoche i responded in Discord but copying here:

i think this is an issue with something in your PATH, like you have gem in your PATH pointing to a wrong directory

can you double check your PATH?

@ShupingHe ShupingHe force-pushed the class-and-sqltable-border-radius branch from 551d91b to 1f39f7e Compare March 8, 2023 08:48
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.

these look great. sorry can you add this test to d2sketch_test as well? I just want to make sure it works there too, then we can merge

@ShupingHe ShupingHe force-pushed the class-and-sqltable-border-radius branch from 1f39f7e to 39419f2 Compare March 9, 2023 01:30
d2renderers/d2svg/table.go Outdated Show resolved Hide resolved
@ShupingHe
Copy link
Contributor Author

@alixander already regenerated some tests. seems like these changes have no effect on d2Sketch, do i need to fix it?

@alixander
Copy link
Collaborator

@alixander already regenerated some tests. seems like these changes have no effect on d2Sketch, do i need to fix it?

no it's not a big deal, as long as not broken

@ShupingHe
Copy link
Contributor Author

ShupingHe commented Mar 9, 2023

@alixander one thing need to discuss with you is, althought these changes work well, but the logic for rendering border-radius is distributed between different files. isn't it hard to maintain?

@alixander
Copy link
Collaborator

@alixander one thing need to discuss with you is, althought these changes work well, but the logic for rendering border-radius is distributed between different files. isn't it hard to maintain?

yeah it's unfortunate. i didn't expect class and table to deviate so much from d2svg's main rendering. eventually we should consolidate in a refactor

@ShupingHe
Copy link
Contributor Author

@alixander still some little changes even i just add rx and ry for rect with non-zero border-radius, is that normal?
image

@alixander
Copy link
Collaborator

@donglixiaoche for the sketch tests? yeah, there's some tiiny precision issue dependent on architecture. what machine are u running? #921

@ShupingHe
Copy link
Contributor Author

image

but i regenerate those testcases anyway, do i need revert it?

@alixander
Copy link
Collaborator

alixander commented Mar 9, 2023

but i regenerate those testcases anyway, do i need revert it?

@donglixiaoche yes please revert it. sorry about that.

as well as the CLI tests: e2etests-cli/testdata/TestCLI_E2E/hello_world_png_sketch.exp.png

@ShupingHe
Copy link
Contributor Author

but i regenerate those testcases anyway, do i need revert it?

@donglixiaoche yes please revert it. sorry about that.

as well as the CLI tests: e2etests-cli/testdata/TestCLI_E2E/hello_world_png_sketch.exp.png

@alixander done, ./make.sh still passed, sry, my bad, i thought it's part of our ci process

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 and works well! thanks for taking such a tedious one. classes and sql_tables are some of the most used shapes people use D2 for, so i'm sure we'll start seeing some usages of these in no time

d2renderers/d2svg/class.go Outdated Show resolved Hide resolved
d2renderers/d2svg/class.go Show resolved Hide resolved
d2renderers/d2svg/table.go Outdated Show resolved Hide resolved
d2renderers/d2svg/table.go Outdated Show resolved Hide resolved
d2renderers/d2svg/table.go Outdated Show resolved Hide resolved
d2renderers/d2svg/class.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.

beautiful (code, commit msgs, and resulting diagrams)!

@alixander alixander merged commit 379e3ae into terrastruct:master Mar 10, 2023
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: border-radius should work with class and sql-table
2 participants