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

Add unfinished-comments feature #3694

Merged
merged 9 commits into from
Nov 3, 2020
Merged

Add unfinished-comments feature #3694

merged 9 commits into from
Nov 3, 2020

Conversation

cheap-glitch
Copy link
Member

Thanks for contributing! 🍄

  1. LINKED ISSUES: Closes Notify the user of unfinished comments in a tab #3680

  2. TEST URLS:

  1. SCREENSHOT:
    Peek 2020-10-31 18-24

Notes:

  • I used the visibilitychange event instead of pageshow/pagehide as the latter weren't working for me in Firefox. As an added bonus, it also works when hiding/minifying the whole browser window.
  • I followed the suggestion of the issue for the notification text, but imho something shorter would be better, e.g. (+) or [*] (akin to what editors show when a file has been modified but not saved).

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Thank you!

source/features/unfinished-comments.tsx Outdated Show resolved Hide resolved
source/features/unfinished-comments.tsx Outdated Show resolved Hide resolved
source/features/unfinished-comments.tsx Outdated Show resolved Hide resolved
@cheap-glitch
Copy link
Member Author

What do you think about using a shorter text for the indicator (e.g. [+])? It hides less of the original tab title — but on the other hand it's also less noticeable.

@fregante
Copy link
Member

I suggested the extended text because anything else doesn’t give enough information about what’s going on, especially since this isn’t a GitHub feature. Ideally you want to go back to the tab as soon as possible and finish the comment anyway.

source/features/unfinished-comments.tsx Outdated Show resolved Hide resolved
source/features/unfinished-comments.tsx Outdated Show resolved Hide resolved
source/features/unfinished-comments.tsx Outdated Show resolved Hide resolved
@fregante fregante self-requested a review October 31, 2020 23:31
@fregante
Copy link
Member

fregante commented Oct 31, 2020

Gifs/screenshots are more helpful when they include some context. You can use this:

@fregante fregante changed the title Notify user of unfinished comments in hidden tabs Add unfinished-comments feature Nov 3, 2020
@fregante fregante merged commit 846350f into refined-github:master Nov 3, 2020
@cheap-glitch cheap-glitch deleted the unfinished-comments branch November 3, 2020 08:57
@fregante
Copy link
Member

fregante commented Nov 5, 2020

This unfortunately also catches comments that are being sent, for example:

  1. Write comment
  2. Click to send it
  3. Immediately switch tab

When switching the tab, the comment will be detected as "draft" but will be sent not long after. I think that there should be an event we can listen to so that when the comment is submitted, the label is removed.

I wonder how this can be solved easily and comprehensively (wait-for-ci for example also will cause this)

@cheap-glitch
Copy link
Member Author

When switching the tab, the comment will be detected as "draft" but will be sent not long after. I think that there should be an event we can listen to so that when the comment is submitted, the label is removed.

We could try listening to the submit event of every form on the page.

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.

Notify the user of unfinished comments in a tab
3 participants