Skip to content

feat: ability to add border-radius to d2 icons #2428

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.

Sign up for GitHub

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

Jump to bottom
Merged
alixander merged 31 commits into from
Mar 30, 2025
Merged

feat: ability to add border-radius to d2 icons #2428

alixander merged 31 commits into from
Mar 30, 2025

Conversation

@melsonic
Copy link
Contributor

@melsonic melsonic commented Mar 16, 2025 •
edited
Loading

Screenshot From 2025-03-16 21-10-41

Final Result

Result


/fixes #2409

alixander
alixander reviewed Mar 16, 2025
View reviewed changes
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.

can you add a test in txtar.txt? for

  1. icons on shapes
  2. icons on connections
  3. icon as an image (even if not done yet)

@melsonic
Copy link
Contributor Author

melsonic commented Mar 16, 2025

3. icon as an image (even if not done yet)

Yep, sure Alex.

Well, there are some failed tests, I will need to work on them too. But created a draft PR to update you.

@melsonic
Copy link
Contributor Author

melsonic commented Mar 17, 2025

Hi @alixander, I have updated the expected files with TESTDATA_ACCEPT=1 go test *the test folder*, now all tests are passing, and git diff has become pretty big now, so I was wondering if it is usual, or am i doing something wrong here?

alixander
alixander reviewed Mar 18, 2025
View reviewed changes
@melsonic melsonic marked this pull request as ready for review March 19, 2025 18:03
@melsonic
Copy link
Contributor Author

melsonic commented Mar 19, 2025 •
edited
Loading

Hey @alixander, I have added omitempty option to some struct fields + some workarounds, now the diffs count is significantly reduced. Also added the recommended tests in txtar.txt. Please review.

Also ./ci/test.sh is passing locally.


image

alixander
alixander requested changes Mar 22, 2025
View reviewed changes
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.

looks great, and nice job navigating the compiler code!

@melsonic
Copy link
Contributor Author

melsonic commented Mar 22, 2025

Please review @alixander

alixander
alixander requested changes Mar 22, 2025
View reviewed changes
@alixander
Copy link
Collaborator

alixander commented Mar 22, 2025

Btw you can just hit this button in github to re-request and I'll get pinged =)

Screenshot 2025-03-22 at 8 54 43 AM

@melsonic
Copy link
Contributor Author

melsonic commented Mar 22, 2025

Btw you can just hit this button in github to re-request and I'll get pinged =)

Screenshot 2025-03-22 at 8 54 43 AM

Okay, Thanks

@melsonic melsonic requested a review from alixander March 22, 2025 19:28
alixander
alixander requested changes Mar 25, 2025
View reviewed changes
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.

nice! few more comments

@melsonic melsonic requested a review from alixander March 25, 2025 17:30
alixander
alixander requested changes Mar 28, 2025
View reviewed changes
@melsonic melsonic requested a review from alixander March 29, 2025 09:10
alixander
alixander requested changes Mar 29, 2025
View reviewed changes
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.

last set of comments, almost ready to merge!

@melsonic melsonic requested a review from alixander March 29, 2025 18:52
alixander
alixander requested changes Mar 29, 2025
View reviewed changes
@melsonic melsonic requested a review from alixander March 30, 2025 10:47
alixander
alixander requested changes Mar 30, 2025
View reviewed changes
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.

looks good to me! Sorry been merging stuff in between, so if you could rebase and regenerate the tests again, it'll be good to go

@melsonic melsonic requested a review from alixander March 30, 2025 14:24
alixander
alixander approved these changes Mar 30, 2025
View reviewed changes
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.

💯 lgtm!

@alixander alixander merged commit 0cec0bc into terrastruct:master Mar 30, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alixander alixander alixander approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

border-radius should work on icon

2 participants

@melsonic @alixander