Skip to content

External Link Checker: Open in new tab #3517

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

Merged

Conversation

luc122c
Copy link
Contributor

@luc122c luc122c commented Oct 13, 2023

When checking the links from #999, I thought it might be nicer for the links to open in a new tab (since they are external to GitHub).

I was also wondering if it's worth truncating the length of the URL to prevent the table from extending past the width of the comment?

@luc122c luc122c added the component-test Affects the automated tests. label Oct 13, 2023
Copy link
Member

@FloEdelmann FloEdelmann left a comment

Choose a reason for hiding this comment

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

it might be nicer for the links to open in a new tab

I don't have a strong opinion on this, but I'm fine with the change.

if it's worth truncating the length of the URL to prevent the table from extending past the width of the comment

Yes, I think that would be a good improvement, since the icons column should always be visible. Or maybe we just have to swap the columns around?

@luc122c
Copy link
Contributor Author

luc122c commented Oct 15, 2023

Or maybe we just have to swap the columns around?

I like that idea better, since no data is lost. I'll do that now.

@luc122c
Copy link
Contributor Author

luc122c commented Oct 16, 2023

@FloEdelmann I really struggled to get the Markdown formatting right with the nowrap column, so I opted to use HTML formatting only. I hope this is OK.

@luc122c
Copy link
Contributor Author

luc122c commented Oct 17, 2023

Thanks, it looks much neater now. Good spot with the getLinkDataFromBody function, I didn't notice that.

Copy link
Member

@FloEdelmann FloEdelmann 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, thanks 🙂

I'll update the description of #999 manually to the new format now, so that the parser doesn't fail.

@FloEdelmann FloEdelmann enabled auto-merge (squash) October 17, 2023 13:11
@FloEdelmann FloEdelmann merged commit 012338d into OpenLightingProject:master Oct 17, 2023
@luc122c luc122c deleted the link-checker-comment-new-tab branch October 17, 2023 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-test Affects the automated tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants