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

Enable native hovercards when linkifying issues #5414

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

kidonng
Copy link
Member

@kidonng kidonng commented Feb 16, 2022

@@ -24,7 +24,6 @@ export function linkifyIssues(
baseUrl: '',
...options,
attributes: {
rel: 'noreferrer noopener',
Copy link
Member Author

Choose a reason for hiding this comment

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

Unnecessary for issue (internal) links

@@ -36,11 +35,13 @@ export function linkifyIssues(
// Enable native issue title fetch
for (const link of linkified.children as HTMLCollectionOf<HTMLAnchorElement>) {
const issue = link.href.split('/').pop();
link.setAttribute('class', 'issue-link js-issue-link tooltipped tooltipped-ne');
Copy link
Member Author

Choose a reason for hiding this comment

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

Without .tooltipped .tooltipped-ne, .js-issue-link becomes meaningless as title tooltips can't display even if they are fetched. But this is what GitHub does and the tooltips are overridden anyway:

// https://github.githubassets.com/assets/app/assets/modules/github/issues/issue-link.ts

// Load issue/PR/discussion references to display the title for the tooltip from the server on hover.
// If hovercards are enabled for the referenced type on this part of the DOM,
// this behavior is bypassed in favor of the hovercard.

Comment on lines -40 to -41
link.dataset.errorText = 'Failed to load issue title';
link.dataset.permissionText = 'Issue title is private';
Copy link
Member Author

Choose a reason for hiding this comment

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

Align with GitHub. Also this works for PRs so "issue" is not very precise.

link.dataset.url = link.href;
link.dataset.id = `rgh-issue-${issue!}`;
link.dataset.hovercardType = 'issue';
Copy link
Member Author

Choose a reason for hiding this comment

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

A small note: if [data-hovercard-type] is omitted (or if it also has [data-insert-type-here-hovercards-enabled]), it will unconditionally active.

Using issue is fine since it works and we can't know what type the conversation is without making a request.

link.dataset.url = link.href;
link.dataset.id = `rgh-issue-${issue!}`;
link.dataset.hovercardType = 'issue';
link.dataset.hovercardUrl = `${link.pathname}/hovercard`;
Copy link
Member Author

Choose a reason for hiding this comment

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

I've said this before but: GitHub's routing is super-duper smart so it works even if the path has a trailing slash.

Copy link
Member

@cheap-glitch cheap-glitch left a comment

Choose a reason for hiding this comment

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

Successfully tested on Firefox

@kidonng kidonng merged commit 4e66998 into main Feb 17, 2022
@kidonng kidonng deleted the enable-native-issue-hovercards branch February 17, 2022 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants