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

feature: supports link on connections #1955

Merged
merged 17 commits into from
Dec 14, 2024

Conversation

danielsuh05
Copy link
Contributor

Connections now support link: (fixes issue #1827)

Some notes:

  • Every single test file changed because all of the class names changed when adding link: "".
  • Links don't work yet on connections in PDFs, not sure exactly how to do that.
  • Check link-on-connections for the test I used.

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.

I'm not sure what adding an empty <a> tag does. There's nothing for users to click and no indication it's clickable.

I was imagining something like this: link on the connection makes the label a hyperlink. So google here would be underlined and be in link-style blue (or purple if clicked). If there's a link and no label, the link becomes the label.

Screenshot 2024-05-29 at 2 21 40 PM

@danielsuh05
Copy link
Contributor Author

Oh I see what you mean. I was trying to figure out how exactly you wanted it implemented so I decided to do this for now and get a review. I'll work on it

@danielsuh05 danielsuh05 requested a review from alixander May 30, 2024 01:06
@danielsuh05
Copy link
Contributor Author

Was this more what you were thinking?

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.

Yup!

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.

oh also i think it's a security issue if the edge label is a URL and the edge link is a URL and they are not the same URL. can you make that a compiler error and add a test for it please? (i'm pretty sure we do this for tooltips and links or something already)

@danielsuh05 danielsuh05 requested a review from alixander May 31, 2024 06:19
@danielsuh05 danielsuh05 requested a review from alixander May 31, 2024 15:36
@danielsuh05 danielsuh05 requested a review from alixander May 31, 2024 19:07
@huyz
Copy link

huyz commented Sep 2, 2024

Hi, what's the status on this PR?

@alixander
Copy link
Collaborator

Oops I did not realize the ball was in my court for reviewing. Sorry @danielsuh05 . I'm not sure if you've moved on. No worries if so I can take over, otherwise will leave a review ASAP

@huyz
Copy link

huyz commented Sep 5, 2024

I hope to see this soon. Links on connections are pretty important. They open up a lot of possibilities for letting diagrams act as jump points to, e.g., code.

@danielsuh05
Copy link
Contributor Author

I'm ok continuing with this issue.

@huyz
Copy link

huyz commented Sep 6, 2024

Thank you @danielsuh05 and @alixander!

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 squash all the implementation commits into one and the test diffs into another? makes it easier to review to just see the diff for the implementation


the conflicts should just be a test rerun after rebasing master

@danielsuh05 danielsuh05 force-pushed the issue1827 branch 2 times, most recently from ce752d4 to 92ea220 Compare September 17, 2024 02:33
@alixander
Copy link
Collaborator

@danielsuh05 Let me know if you'd like me to take over

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.

i rebased and made minor fixes, thank you again @danielsuh05

@alixander alixander merged commit 557477a into terrastruct:master Dec 14, 2024
3 checks passed
@bo-ku-ra
Copy link
Contributor

bo-ku-ra commented Feb 5, 2025

is the font color blue only?

@alixander
Copy link
Collaborator

yes, since that's the universal link color. But if you want it different, please make an issue

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.

4 participants