-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Disable quick-mention
on reviews without a comment
#5117
Conversation
source/features/quick-mention.tsx
Outdated
@@ -59,6 +59,11 @@ function init(): void { | |||
<ReplyIcon/> | |||
</button>, | |||
); | |||
|
|||
const timelineItem = avatar.closest('.js-timeline-item')!; | |||
if (timelineItem && !timelineItem.querySelector('.timeline-comment')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timelineItem
doesn't exist for the first "comment" (i.e. author post)
I'd just exclude reviews honestly 😅 like in It's weird that this changes the spacing |
Yeah that makes sense. |
quick-mention
items without a commentquick-mention
on reviews without a comment
Because you don't have write access and thus that info line. |
source/features/quick-mention.tsx
Outdated
// The hovercard attribute avoids `highest-rated-comment` | ||
// Avatars next to review events aren't wrapped in a <div> #4844 | ||
const avatars = select.all(`:is(div.TimelineItem-avatar > [data-hovercard-type="user"]:first-child, a.TimelineItem-avatar):not([href="/${getUsername()!}"], .rgh-quick-mention)`); | ||
const avatars = select.all(`:is(${[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably extract this array instead to avoid (`:is(${[
Common is
-less example:
const avatarSelector = [
].map(x => x + ':not()');
const avatars = select.all(avatarSelector);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also move this to the lint PR if you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest commit did not match this, let's merge this PR without it and you can make the suggested change in the lint PR if desired.
Co-authored-by: Federico Brigante <me@fregante.com>
See screenshots for the issue
Also, as you can see from the screenshot, there is a
z-index
issue happening. However it should not happen again with this patch, but nevertheless I will leave the fix here for completeness:refined-github/source/features/quick-mention.tsx
Line 48 in 2d70121
Add a
style={{zIndex: 2}}
would be fineTest URLs
Screenshot
Before:
After: