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.

Sign up for GitHub

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

Jump to bottom

quick-reactions - New feature #7247

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+106 −0
Draft

quick-reactions - New feature #7247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4797363
optimistic reactions
brumm Feb 4, 2024
17e8ae1
fix eslint warnings
brumm Feb 4, 2024
363c8d5
add test urls
brumm Feb 5, 2024
2bd80ad
add newline at end of file
brumm Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions build/__snapshots__/features-meta.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,11 @@
"description": "Adds a link to create issues from anywhere in a repository.",
"screenshot": "https://github-production-user-asset-6210df.s3.amazonaws.com/1402241/274816033-820ec518-049d-4248-9f8a-27b9423e350b.png"
},
{
"id": "quick-reactions",
"description": "Shows a reaction optimistically",
"screenshot": "https://user-images.githubusercontent.com/1402241/236628453-8b646178-b838-44a3-9541-0a9b5f54a84a.png"
brumm marked this conversation as resolved.
Show resolved Hide resolved
},
{
"id": "quick-repo-deletion",
"description": "Lets you delete your repos in a click, if they have no stars, issues, or PRs.",
Expand Down
1 change: 1 addition & 0 deletions build/__snapshots__/imported-features.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ quick-file-edit
quick-label-removal
quick-mention
quick-new-issue
quick-reactions
quick-repo-deletion
quick-review
quick-review-comment-deletion
Expand Down
1 change: 1 addition & 0 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ Thanks for contributing! 🦋🙌
### Reading comments

- [](# "reactions-avatars") 🔥 [Adds reaction avatars showing _who_ reacted to a comment.](https://user-images.githubusercontent.com/1402241/236628453-8b646178-b838-44a3-9541-0a9b5f54a84a.png)
- [](# "quick-reactions") [Shows a reaction optimistically](https://user-images.githubusercontent.com/1402241/236628453-8b646178-b838-44a3-9541-0a9b5f54a84a.png)
- [](# "embed-gist-inline") [Embeds short gists when linked in comments on their own lines.](https://user-images.githubusercontent.com/1402241/152117903-80d784d5-4f43-4786-bc4c-d4993aec5c79.png)
- [](# "comments-time-machine-links") Adds links to [browse the repository](https://github-production-user-asset-6210df.s3.amazonaws.com/83146190/252749373-9313f1d9-3d92-44a2-a1d1-2b49a29e6a5c.png) and [linked files](https://github-production-user-asset-6210df.s3.amazonaws.com/83146190/252749616-085103bf-17be-4a7d-b63c-aa5003de6dff.png) at the time of each comment.
- [](# "show-names") [Adds the real name of users by their usernames.](https://github-production-user-asset-6210df.s3.amazonaws.com/83146190/252756294-94785dc2-423e-498c-939a-359a012036e0.png)
Expand Down
98 changes: 98 additions & 0 deletions source/features/quick-reactions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import delegate, {DelegateEvent} from 'delegate-it';
import React from 'dom-chef';
import * as pageDetect from 'github-url-detection';

import features from '../feature-manager.js';

const emojiSortOrder = ['THUMBS_UP', 'THUMBS_DOWN', 'LAUGH', 'HOORAY', 'CONFUSED', 'HEART', 'ROCKET', 'EYES'];

function optimisticReactionFromPost(event: DelegateEvent<MouseEvent>): void {
const reaction = event.delegateTarget as HTMLButtonElement;
Comment on lines +9 to +10
Copy link
Member

Choose a reason for hiding this comment

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

IIRC you can just pass the type here, no need to assert it. Refer to its docs if it fails, also fix it elsewhere

Suggested change
function optimisticReactionFromPost(event: DelegateEvent<MouseEvent>): void {
const reaction = event.delegateTarget as HTMLButtonElement;
function optimisticReactionFromPost(event: DelegateEvent<MouseEvent, HTMLButtonElement>): void {

updateReaction(reaction);
}

function optimisticReactionFromMenu(event: DelegateEvent<MouseEvent>): void {
const reactionButton = event.delegateTarget as HTMLButtonElement;
const reactionsMenu = reactionButton.closest('reactions-menu')!;
const details = reactionsMenu.querySelector('details');
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace every querySelector usage with $ from select-dom? The only exception is when the line has multiple traversing, e.g. x.closest().querySelector('a') is still more readable than $('a', x.closest())

details!.open = false; // Close reactions menu immediately

const reactionsContainer = reactionsMenu.nextElementSibling!.querySelector('.js-comment-reactions-options')!;
Copy link
Member

Choose a reason for hiding this comment

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

Positional traversing makes the code a little more brittle because a simple span in between will break it. If possible, select the parent element instead (e.g. menu.parentElement.querySelector) or just change what reactionsMenu to select its parent instead

const reactionButtonValue = reactionButton.getAttribute('value')!;
Copy link
Member

Choose a reason for hiding this comment

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

Are you looking for reactionButton.value?

const existingReaction = reactionsContainer.querySelector<HTMLButtonElement>(`[value="${reactionButtonValue}"]`);
Copy link
Member

Choose a reason for hiding this comment

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

If the selector includes the tag name, you probably won't need to specify the type

Suggested change
const existingReaction = reactionsContainer.querySelector<HTMLButtonElement>(`[value="${reactionButtonValue}"]`);
const existingReaction = $(`button[value="${reactionButtonValue}"]`, reactionsContainer);


if (existingReaction) {
// The user is updating an existing reaction via the menu, just update the reaction
updateReaction(existingReaction);
} else {
// Reactions have a specific order, so we need to insert the new reaction in the correct position
Copy link
Member

Choose a reason for hiding this comment

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

If:

  • GitHub still updates the UI later
  • it simplifies this piece of code

Please just append the reaction instead, it will update within a second anyway. I'd rather have a feature that lasts longer than one that looks absolutely perfect for only one month.

const reactionIndex = emojiSortOrder.findIndex(item => reactionButtonValue.startsWith(item));
const insertionIndex = [...reactionsContainer.querySelectorAll('button')].findIndex(button => {
const index = Number.parseInt(button.getAttribute('data-button-index-position')!, 10);
return index > reactionIndex;
});
const referenceNode = reactionsContainer.querySelectorAll('button').item(insertionIndex);
const emojiElement = reactionButton.querySelector('g-emoji')!.cloneNode(true);
emojiElement.className = 'social-button-emoji';
Copy link
Member

Choose a reason for hiding this comment

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

What is this class doing and why is it not there already?


reactionsContainer.insertBefore(
Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-modern-dom-apis.md

<>
<button
name="input[content]"
value={reactionButtonValue}
type="submit"
className="social-reaction-summary-item btn-link d-flex no-underline flex-items-baseline mr-2 user-has-reacted"
>
{emojiElement}
<span className="js-discussion-reaction-group-count">1</span>
</button>
<span/> {/* helps avoid unwanted margin due to adjacent siblings of .social-reaction-summary-item */}
</>,
referenceNode,
);
}
}

function updateReaction(buttonElement: HTMLButtonElement): void {
const countElement = buttonElement.querySelector(
'.js-discussion-reaction-group-count',
)!;
const count = Number.parseInt(countElement.textContent, 10);
Copy link
Member

Choose a reason for hiding this comment

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

Probably also works

Suggested change
const count = Number.parseInt(countElement.textContent, 10);
const count = Number(countElement.textContent);

const isIncrease = !buttonElement.getAttribute('value')!.endsWith('unreact');
const newCount = isIncrease ? count + 1 : count - 1;

if (newCount === 0) {
buttonElement.setAttribute('hidden', '');
} else if (newCount > count) {
countElement.textContent = String(newCount);
buttonElement.classList.add('user-has-reacted');
buttonElement.classList.remove('color-fg-muted');
} else {
countElement.textContent = String(newCount);
buttonElement.classList.add('color-fg-muted');
}
}

function init(signal: AbortSignal): void {
delegate('reactions-menu + form button[value$=react]', 'click', optimisticReactionFromPost, {signal});
delegate('reactions-menu button[value$=react]', 'click', optimisticReactionFromMenu, {signal});
}

void features.add(import.meta.url, {
include: [
pageDetect.hasComments,
pageDetect.isReleasesOrTags,
pageDetect.isSingleReleaseOrTag,
pageDetect.isDiscussion,
],
init,
});

/*

Test URLs:

https://github.com/brumm/tako/issues/27#issuecomment-617314195
https://github.com/vercel/next.js/discussions/40684#discussioncomment-3689167

*/
1 change: 1 addition & 0 deletions source/refined-github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ import './features/jump-to-change-requested-comment.js';
import './features/esc-to-cancel.js';
import './features/easy-toggle-files.js';
import './features/quick-repo-deletion.js';
import './features/quick-reactions.js';
import './features/clean-repo-sidebar.js';
import './features/rgh-feature-descriptions.js';
import './features/archive-forks-link.js';
Expand Down