Add `comment-on-draft-pr-indicator` feature #4520

darkred · 2021-06-26T21:05:32Z

Test URLs

Any draft PR, e.g.
https://github.com/darkred/test/pull/6

Screenshot

Note to reviewers:

The selector for the "Draft" label, #partial-discussion-header span[title="Status: Draft"] ,
matches twice in draft PR pages (but none in non-draft ones, of course).
Do I have to make it more specific, nevertheless?

yakov116 · 2021-06-27T04:23:05Z

The selector for the "Draft" label, #partial-discussion-header span[title="Status: Draft"] ,

There is a detection for isDraft do

exclude: [
	() => !isDraft
]

fregante · 2021-06-27T05:11:21Z

It should also cover the Files tab, where comments on temporary code are more likely to happen.

source/features/comment-to-draft-pr-button.tsx

- Followed all suggestions by yakov116 and fregante - Updated the feature screenshot to include the Files tab

yakov116 · 2021-06-27T15:38:41Z

source/features/comment-to-draft-pr-button.tsx

+	let commentButton;
+	if (pageDetect.isPRConversation()) {
+		commentButton = select('#partial-new-comment-form-actions .btn-primary');
 		commentButton!.innerHTML = 'Comment to Draft PR';


Suggested change

let commentButton;

if (pageDetect.isPRConversation()) {

commentButton = select('#partial-new-comment-form-actions .btn-primary');

commentButton!.innerHTML = 'Comment to Draft PR';

if (pageDetect.isPRConversation()) {

select('#partial-new-comment-form-actions .btn-primary')!.textContent = 'Comment to Draft PR';

Can we use textContent?

yes, but, e.g. for the Conversation tab as in here,
the 'Comment' button textContent is:

\n \n \n Comment\n\n \n\n

and so I'd have to use .replace('Comment, Comment to Draft PR')
instead of putting fixed string (innerHTML = ' Comment to Draft PR ' ).

Are you ok with that?

Its fine don't worry about the '\n'. I just tried it it will work correctly

Yakov is not suggesting to use replace. Just use = and it works. The whitespace is useless.

Also never use innerHTML because it's flagged by the extension stores, just use textContent

Thanks, I just did.

Btw, I feel once again embarrassed making the same mistakes over and over again..
Could I ask please to add some extra notes/warnings in https://github.com/sindresorhus/refined-github/blob/main/contributing.md to make sure people like me always follow them?

What comes to mind from your reviews to my previous PRs:

describe the preferable way to sort imports for this repo

never use innerHTML because it's flagged by the extension stores. Just use textContent.

prefer select(), not querySelector

with select(), when having multiple selectors, split them to multiple lines

selectors should be short and smart/concise. Use those class and attributes combinations that are as much specific as possible and don't need almost any other specifiers.

use ternary if when possible.

Do you think these "tips" can be useful to contributors, or they are too self-explanatory/beginner level?

I could provide a PR with these of course, but I need your opinion first.

Don't worry too much about these small mistakes. If the PR is salvageable you can learn about the project’s code style over time.

Some of those rules could be part of the contributor guide, but I'd rather have eslint check for them. It's weird that innerHTML isn't already caught by eslint, e.g.

https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-dom-node-text-content.md
https://github.com/salesforce/eslint-plugin-lwc/blob/master/docs/rules/no-inner-html.md

fregante · 2021-06-27T16:51:50Z

source/features/comment-to-draft-pr-button.tsx

+		commentButton = select('#partial-new-comment-form-actions .btn-primary');
+		commentButton!.innerHTML = 'Comment to Draft PR';
+	} else if (pageDetect.isPRFiles()) {
+		delegate('.diff-table', 'button', 'click', handleClicks);


I don't think that's the right way. We add custom buttons like table-input on the comment forms on the Files tab. You can copy what they use

Sorry, but I don't quite understand.

With this feature, the purpose is to simply change the name of existing buttons,
not (remove the existing ones and) add new buttons with a different name.
Are you suggesting to use .remove() and .after()?

It looks like a plain loop is all that should be needed and GitHub will preserve the changes, no need for onNewComments at all:

https://github.com/sindresorhus/refined-github/blob/ae07d212f364ba0193958b95729e4e99d4653b90/source/features/table-input.tsx#L39-L40

Regarding onNewComments:
I tried removing it, but then, in 'Conversations' tab, the 'Comment' button name no longer changes after submitting a new comment. Therefore it seems necessary for the 'Conversations' tab.

Then, I also tried changing

commentButton = select('#partial-new-comment-form-actions .btn-primary'); commentButton!.textContent = 'Comment to Draft PR';

to

commentButton = select.all('#partial-new-comment-form-actions .btn-primary'); for (const element of commentButton) { element.textContent = 'Comment to Draft PR'; }

but didn't make any difference.

And regarding my use of delegate:
in 'Files 'tab, e.g. https://github.com/darkred/test/pull/6/files, initially, there's no comment textarea,
and no table:not([style="display: none"]) .review-simple-reply-button/.start-review-label button elements:
these elements only exist after you click a '+' in a row (capture)
therefore I think it's necessary to delegate clicks on the .diff-table element.
You can't loop on these elements, before they exist.

Am I not right? Have I got something wrong?

table-input does not have :not([style="display: none"]) because it's not needed. Same goes for onNewComments in this feature, it shouldn't be needed.

As I tried saying before, there's a "template comment box element" that GitHub clones every time it's needed, including all the changes we make to it. Your selector explicitly excludes this template element. This is also why onNewComments is very likely not needed.

Maybe it needs dontDeduplicate. (I forgot what we called it)

For the 'Files' tab, onNewComments is indeed not needed:
I had used the :not([style="display: none"]) selector part
because I wrongly thought that these hidden elements shouldn't also match my selector.
It indeed works ok after removing that selector part + onNewComments.
I just removed the selector part.

For the Conversations tab, your are most probably right that onNewComments is not needed as well,
but I tried using any of the selectors below with:

if (pageDetect.isPRConversation()) { const buttonsComment = select.all('a selector'); for (const element of buttonsComment) { element.textContent = 'Comment on draft PR'; } }

.timeline-comment .btn-primary

.form-actions .btn-primary

form .btn-primary

button[type="submit"].btn-primary

but unfortunately it still doesn't work after submitting a new comment.

I also tried searching in devtools Inspector for strings such as "Leave a comment" (for the textarea)
or for "Close pull request" (for the button)
hoping to discover such a hidden 'template comment box element' (and get the 'Submit' button selector from inside it), but didn't find anything.

Also, I tried adding the following inside void features.add( but it didn't help:

deduplicate: 'false',

Regarding the Conversations tab, maybe this hidden 'template comment box element' contains code only for the editor's toolbar buttons (a button of which, md-task-list, is used as reference in table-input),
but not for the 'Comment' button (or the 'Close pull request' one) too?

I'm claiming this because I couldn't find any such hidden additional button, as I describe in my previous comment.

orrc · 2021-06-28T13:47:40Z

Just a small note as a native English speaker… 😃

I would suggest that the prepositions need to be changed ("to" versus "on"), and I would argue that "draft" rather than "Draft" should be used.

"Draft" is being used as an adjective, it looks a bit nicer as lowercase within a sentence (IMO), and that's how GitHub uses it in their docs and UI for sentences, e.g. "Convert to draft", or "… mark a draft pull request as ready …", plus also elsewhere in Refined GitHub.

Current text in this PR	Proposed text
Add single comment to Draft PR	Add single comment to draft PR
Start a review to Draft PR	Start a review on draft PR
Comment to Draft PR	Comment on draft PR

darkred · 2021-06-28T15:14:46Z

Thanks for your comment.

Regarding on --> to, thanks for noting, yes, as verb it's "comment on/upon/about", there's no "comment to" . I'll change it.

Regarding draft PR --> Draft PR, indeed it looks nicer as lowercase,
but I'd argue that, since the gist of this feature is whether the PR is draft,
having it in uppercase, i.e. Draft, would add emphasis.
I'd like to ask for the reviewers opinion, please.

source/features/comment-to-draft-pr-button.tsx

Co-authored-by: Federico Brigante <me@fregante.com>

…ed/refined-github into comment-to-draft-pr-button

Rename the feature

darkred · 2021-06-28T17:48:55Z

With the new commits, I:

followed both suggestions by @orrc ,
renamed the feature + updated the screenshot, and
removed the unnecessary table:not([style="display: none"]) selector part.

darkred · 2021-07-06T17:14:11Z

@fregante
I see you closed this PR 😞 Isn't it salvageable??
It's in functioning state - what I cannot find (and I think it doesn't exist) is a hidden 'template' for the submit buttons,
that would make onNewComments unnecessary.

fregante · 2021-07-06T17:17:59Z

You need to follow the reviews if you want to open PRs here, otherwise we're just wasting time.

What I suggested works exactly like it works for table-input. There's one situation where it doesn't work for either one and that doesn't even matter because it only happens after a review started and the user is leaving a comment on the same thread.

darkred · 2021-07-06T17:32:04Z

I followed all reviews, with the only exception of managing to remove the need for onNewComments.

The way your presented the alternative approach, appeared to me as a probability, not a certainty:

It looks like a plain loop is all that should be needed and GitHub will preserve the changes, no need for onNewComments at all:

This is also why onNewComments is very likely not needed.

and, after my various attempts (with my limited experience compared to you), I came to the result that it wasn't possible.

I'm sorry I couldn't make this effort to completion.

darkred · 2021-07-08T14:13:41Z

@fregante
Could you please help me clarify one last thing, about how to properly contribute to the repo?

You said above that:

You need to follow the reviews if you want to open PRs here, otherwise we're just wasting time.

Also, in your new PR you have this commit message b385a84 :

Do what the reviewers say

In that commit, the only significant change is that you removed the onNewComments listener.

I would never disregard/ignore/argue with the reviewers.
You are way more experienced than me, your reviews can only be helpful and instructional.

With your last review, you suggested that use onNewComments can be avoided.
Unfortunately, despite my efforts, although I tried as much as I could, I couldn't make it work (the button wouldn't stay after updating). With my comments 1, 2, 3 I just wanted to describe my failed efforts.

Yes, I admit my comments are sometimes too verbose/detailed.
Comparing them to your terse style of commenting (yours and the other collaborators),
I understand that they might appear tedious to read them through, to such experienced people like you,
but that's my way of expressing, I want to provide as much details as possible.

So, I just wanted to ask you this:
Did you close the PR because you thought I disregard the reviewers (your review)?
Would you prefer me, instead of writing all these comments explaining my failed efforts,
to just immediately remove the onNewComments listener via a new commit,
and briefly say the following:

without onNewComments the button won't stay after updating,
and setting deduplicate: false didn't help.
I can't find a way to make it work without onNewComments. Please help/suggest ideas.

But, wouldn't we end up to the same dead-end like in this previous closed PR of mine #4123 (comment)
i.e. that you would still have to close the PR:

If you can't finish the PR autonomously you're asking us to figure it out with you, which means that the PR isn't useful to us, sorry.

I see that you made a lot of improvements from your commit 6a30be8 and on,
making use of a lightweight listener instead of onNewComments to catch dynamic "Add comment",
something I couldn't achieve myself (I was unaware of onReplacedElement).

I am asking you all this, because I want to have clear in my mind how to properly contribute to your repo,
I'd hate to be a burden, or waste any of your time - I only want to contribute.

fregante · 2021-07-08T17:29:36Z

This was mostly because in this PR a few things had to be repeated several times before being applied, with your focus going on the wrong part of our suggestion. For example you used replace() when Yakov gave you an exact code suggestion that would have just worked, and the table-input suggestion that should have resulted in the thought: “table-input works, why doesn’t it work for the button?” instead of “I doesn’t work for the button, I can’t use it.”

Perhaps the easiest way to go about it is to make the requested changes and then show how they don't work in a gif. This way we can see whether the suggestion was attempted correctly. For example if you implemented the loop with the broken selector my suggestion would have been to just drop the selector and it would have worked most of the time.

Occasionally suggestions will be wrong though, but that’s the problem with commenting on code without running it. The other problem is that once I have to deal with the code myself, by running it and figure it out, it’s easier to just send a PR myself. And that’s ok sometimes, we can’t merge every PR.

Regarding my “probability” suggestion: 😃

Also it would personally help me read your comments if you didn’t add a line break after commas 😂 as that is not customary in English. But that’s just my opinion, man 😜

darkred · 2021-07-08T17:52:34Z

Thanks for replying.

Yes, my English is far from perfect, and, yes, there can be misunderstanding in a discussion when the one is not a native speaker. But I think that most of the time the other understands the gist of what I'm trying to say.

And about my linebreaks after commas: I know it's not necessary, but I don't even realize that I put commas. The point for me is to have linebreaks, as it seems to me that it makes the text easier to read. I guess I'm wrong, I'll try to avoid it.

fregante · 2021-07-08T19:48:45Z

my English is far from perfect

Don't say that at all! Your english is 💯 !

it seems to me that it makes the text easier to read

Yeah it's common among programmers, generally I prefer flowing text since the browser will pick the best place to wrap lines.

fregante · 2021-07-08T19:53:57Z

Related meme

fregante · 2021-07-08T20:15:31Z

By the way if you don't mind me asking, are you from Greece?

darkred · 2021-07-08T20:40:06Z

Yes, I am from Greece. Neighbor of Italy 🙂

Add comment-to-draft-pr-button feature

1ab99f1

darkred mentioned this pull request Jun 26, 2021

Indicator that you're about to comment on a Draft PR #3857

Closed

yakov116 added the enhancement label Jun 27, 2021

fregante marked this pull request as draft June 27, 2021 05:12

fregante marked this pull request as ready for review June 27, 2021 05:12

fregante marked this pull request as draft June 27, 2021 05:12

fregante reviewed Jun 27, 2021

View reviewed changes

source/features/comment-to-draft-pr-button.tsx Outdated Show resolved Hide resolved

darkred added 2 commits June 27, 2021 18:23

Followed all suggestions + updated screenshot

be36f7f

- Followed all suggestions by yakov116 and fregante - Updated the feature screenshot to include the Files tab

lint

74918da

darkred marked this pull request as ready for review June 27, 2021 15:30

yakov116 reviewed Jun 27, 2021

View reviewed changes

fregante marked this pull request as draft June 27, 2021 16:48

fregante reviewed Jun 27, 2021

View reviewed changes

Replace innerHTML with textContent

eb31f43

fregante reviewed Jun 28, 2021

View reviewed changes

source/features/comment-to-draft-pr-button.tsx Outdated Show resolved Hide resolved

darkred and others added 5 commits June 28, 2021 18:29

Update source/features/comment-to-draft-pr-button.tsx

bb9e5fe

Co-authored-by: Federico Brigante <me@fregante.com>

Change 'to' to 'on', and 'Draft' to 'draft'

f094726

Merge branch 'comment-to-draft-pr-button' of https://github.com/darkr…

b7c9a2f

…ed/refined-github into comment-to-draft-pr-button

Rename the feature

6a3fea8

Rename the feature

Remove unnecessary selector

0f8549a

lint

f0eba2d

darkred changed the title ~~Add comment-to-draft-pr-button feature~~ Add comment-on-draft-pr-rename-buttons feature Jun 28, 2021

darkred added 2 commits June 28, 2021 20:58

Update readme.md

6fe1e34

Rename the feature one last time + improve description

74b1632

darkred changed the title ~~Add comment-on-draft-pr-rename-buttons feature~~ Add comment-on-draft-pr-indicator feature Jun 28, 2021

fregante mentioned this pull request Jun 29, 2021

preview-hidden-comments includes the whole comment in the preview #4524

Closed

fregante closed this Jul 6, 2021

fregante mentioned this pull request Jul 6, 2021

Add comment-on-draft-pr-indicator feature #4541

Merged

darkred deleted the comment-to-draft-pr-button branch July 6, 2021 17:35

darkred restored the comment-to-draft-pr-button branch July 7, 2021 09:28

darkred mentioned this pull request Jul 7, 2021

Expand prefer-dom-node-text-content against .innerHTML or implement no-inner-html rule sindresorhus/eslint-plugin-unicorn#1405

Open

darkred deleted the comment-to-draft-pr-button branch July 8, 2021 14:39

cheap-glitch mentioned this pull request Oct 12, 2021

Fix preview-hidden-comments on review comments #4915

Merged

Uh oh!

Add comment-on-draft-pr-indicator feature #4520

Add comment-on-draft-pr-indicator feature #4520

Uh oh!

Conversation

darkred commented Jun 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test URLs

Screenshot

Note to reviewers:

Uh oh!

yakov116 commented Jun 27, 2021

Uh oh!

fregante commented Jun 27, 2021

Uh oh!

Uh oh!

yakov116 Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkred Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yakov116 Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

fregante Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkred Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

fregante Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

darkred Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

darkred Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yakov116 Jun 28, 2021

Choose a reason for hiding this comment

Uh oh!

darkred Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darkred Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orrc commented Jun 28, 2021

Uh oh!

darkred commented Jun 28, 2021

Uh oh!

Uh oh!

darkred commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darkred commented Jul 6, 2021

Add `comment-on-draft-pr-indicator` feature #4520

Add `comment-on-draft-pr-indicator` feature #4520

darkred commented Jun 26, 2021 •

edited

Loading

yakov116 Jun 27, 2021 •

edited

Loading

darkred Jun 27, 2021 •

edited

Loading

fregante Jun 27, 2021 •

edited

Loading

darkred Jun 27, 2021 •

edited

Loading

darkred Jun 27, 2021 •

edited

Loading

darkred Jun 27, 2021 •

edited

Loading

fregante Jun 28, 2021 •

edited

Loading

darkred Jun 28, 2021 •

edited

Loading

darkred Jul 5, 2021 •

edited

Loading

darkred commented Jun 28, 2021 •

edited

Loading

darkred commented Jul 6, 2021 •

edited

Loading

darkred commented Jul 8, 2021 •

edited

Loading