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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add jump-to-conversation-close-event feature #5794

Merged
merged 21 commits into from
Jul 11, 2022
Merged

Add jump-to-conversation-close-event feature #5794

merged 21 commits into from
Jul 11, 2022

Conversation

yakov116
Copy link
Member

@yakov116 yakov116 commented Jul 7, 2022

My first add feature PR in a year! Last one was #4534. 馃帀

Closes #4773

Test URLs

Closed Issue: refined-github/sandbox#24
Closed Issue (Not Planned): refined-github/sandbox#2
Merged PR: refined-github/sandbox#23
Closed PR: refined-github/sandbox#22

Screenshot

Video_2022-07-07_095050

@yakov116 yakov116 marked this pull request as draft July 7, 2022 14:15
@yakov116
Copy link
Member Author

yakov116 commented Jul 7, 2022

Missing Status: Closed as not planned

constructor: HTMLSpanElement,
add(messageContainer) {
messageContainer.classList.add('rgh-jump-to-conversation-close-event');
// Hide the native title
Copy link
Member Author

@yakov116 yakov116 Jul 7, 2022

Choose a reason for hiding this comment

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

@fregante is this a good idea? We cannot modify the title since that is our detection, but I don't want the title "Status: Closed" tooltip to show.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it's not particularly useful, but let's not mess with it

Copy link
Member Author

Choose a reason for hiding this comment

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

It gets in the way of the other tooltip

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 extend the comment explaining why we need to leave the title attribute, why we can鈥檛 modify it, and why it needs to be hidden at all?

The current comment only explain what it鈥檚 doing but not why

@fregante
Copy link
Member

fregante commented Jul 7, 2022

My first add feature PR in a year!

yes-napoleon-dynamite

Welcome back!

@yakov116
Copy link
Member Author

yakov116 commented Jul 7, 2022

Welcome back!

Thanks! I moved, my computer broke and things just did not settle down until now.
Hope to be more active now.

@kidonng

This comment was marked as resolved.

@yakov116

This comment was marked as resolved.

@fregante

This comment was marked as resolved.

@kidonng

This comment was marked as resolved.

@fregante

This comment was marked as resolved.

readme.md Outdated Show resolved Hide resolved
yakov116 and others added 3 commits July 7, 2022 13:28
Co-authored-by: Flo Edelmann <florian-edelmann@online.de>
@yakov116 yakov116 marked this pull request as ready for review July 7, 2022 17:48
@yakov116
Copy link
Member Author

yakov116 commented Jul 7, 2022

Note this will fail on big PR such as #1986

But see #3813 (comment) so I don't see an issue

@fregante

This comment was marked as resolved.

yakov116 and others added 2 commits July 8, 2022 06:04
Co-authored-by: Federico Brigante <me@fregante.com>
Co-authored-by: Federico Brigante <me@fregante.com>
@yakov116 yakov116 requested a review from fregante July 10, 2022 14:41
Co-authored-by: Federico Brigante <me@fregante.com>
@yakov116 yakov116 merged commit c4fbe2b into main Jul 11, 2022
@yakov116 yakov116 deleted the jump-event branch July 11, 2022 12:48
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.

Jump to Issue/PR closed events
5 participants