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

feat(ui): Make URLs clickable in workflow node info #13494

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

Adrien-D
Copy link
Member

No linked issue as it's a very small quick win, not sure it needs discussions

Motivation

In workflow node details we may have templates that output links to other systems or other workflows. So that would be great to make navigation easier by showing links instead of text.

Modifications

I implemented linkify-it library to find url matches and wrap those in a link tag.

Before After
Screenshot 2024-08-22 at 11 41 20 Screenshot 2024-08-22 at 11 41 01
Screenshot 2024-08-22 at 11 41 24 Screenshot 2024-08-22 at 11 41 05

Verification

I tested of several workflow to make sure everything is fine

@Adrien-D Adrien-D marked this pull request as ready for review August 22, 2024 10:00
@agilgur5 agilgur5 changed the title feat: Make URLs clickable in workflow node info feat(ui): Make URLs clickable in workflow node info Aug 22, 2024
Copy link
Contributor

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oo I like the table! A few considerations below

@@ -68,10 +69,40 @@ interface Props {
onResume?: () => void;
}

const linkify = new LinkifyIt();

function linkifyText(text: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a util function given that it's likely to be used in other places.

That also raises the question: is this the only place we'd add this? Does this open too many floodgates?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No you're right we could definitely use this in other places, can't see where atm but I'll make it easily reusable

ui/package.json Show resolved Hide resolved
@tooptoop4
Copy link
Contributor

do u think same approach would work on #13148 ?

@agilgur5
Copy link
Contributor

No, since that uses a terminal with xterm. If we simplified it as I mentioned there, then it could work depending on the approach

Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@isubasinghe isubasinghe merged commit 6fa5365 into argoproj:main Oct 21, 2024
16 checks passed
@agilgur5 agilgur5 added this to the v3.6.0 milestone Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants