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

shorten-links processes way more links than required #5889

Closed
fregante opened this issue Aug 5, 2022 · 2 comments · Fixed by #5962
Closed

shorten-links processes way more links than required #5889

fregante opened this issue Aug 5, 2022 · 2 comments · Fixed by #5962
Labels
bug help wanted small Issues that new contributors can pick up

Comments

@fregante
Copy link
Member

fregante commented Aug 5, 2022

Description

As the selector suggests, this feature is being applied to all the links on every page:

observe(`a[href]:not(.${linkifiedURLClass})`, {

We should look into the feature’s git history to see where this feature is expected to work exactly (e.g. only on markdown, likely) and make the selector more specific, perhaps even avoiding known-misses like these 2 links:

Natively shortened

#5888

Natively extended

How to replicate the issue

  1. Visit https://github.com/refined-github/refined-github/issues
  2. I don't think any links would ever need to be shortened on this page, but the feature is run on 258 links

Extension version

any

Browser(s) used

any

@fregante fregante added bug help wanted meta Related to Refined GitHub itself small Issues that new contributors can pick up and removed meta Related to Refined GitHub itself small Issues that new contributors can pick up labels Aug 5, 2022
@fregante
Copy link
Member Author

fregante commented Aug 5, 2022

This is because we have a filter inside the callback:

export function shortenLink(link: HTMLAnchorElement): void {
// Exclude the link if the closest element found is not `.comment-body`
// This avoids shortening links in code and code suggestions, but still shortens them in review comments
// https://github.com/refined-github/refined-github/pull/4759#discussion_r702460890
if (link.closest(`${codeElementsSelector}, .comment-body`)?.classList.contains('comment-body')) {

I'm not sure if we can safely merge this given the history of this solution:

I don’t think :has() would be of help with that specific logic either. Related:

@fregante
Copy link
Member Author

fregante commented Aug 5, 2022

I think we can still inline that selector before a[href], without altering the above logic. Like:

- observe(`a[href]:not(.${linkifiedURLClass})`)
+ observe(`:is(${codeElementsSelector}, .comment-body) a[href]:not(.${linkifiedURLClass})`)

This would limit the amount of times the callback is called.

Or maybe it's just this 🤔

- observe(`a[href]:not(.${linkifiedURLClass})`)
+ observe(`.comment-body a[href]:not(.${linkifiedURLClass})`)

@fregante fregante added the small Issues that new contributors can pick up label Aug 5, 2022
@fregante fregante self-assigned this Aug 5, 2022
@fregante fregante removed their assignment Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted small Issues that new contributors can pick up
Development

Successfully merging a pull request may close this issue.

1 participant