Restore `one-click-review-submission` feature #6964

RobHannay · 2023-09-27T21:24:48Z

Fixes #6963.

GitHub have changed their review submission modal so that it:

now has the radio buttons properly nested in a form
the radios' labels are not adjacent siblings anymore

Test URLs

This PR??

Screenshot

Capture d’écran 2023-09-27 à 10 21 11 PM

Enregistrement.de.l.ecran.2023-09-27.a.10.31.39.PM.mov

Description tooltips

…hat modal

source/features/one-click-review-submission.tsx

RobHannay · 2023-09-27T21:30:02Z

source/features/one-click-review-submission.tsx

@@ -43,18 +42,19 @@ function replaceCheckboxes(originalSubmitButton: HTMLButtonElement): void {
 			classes.push('tooltipped tooltipped-nw tooltipped-no-delay');
 		}

+		const label = document.querySelector(`label[for="${radio.id}"]`)?.textContent;


The labels are not adjacent siblings anymore, so I'm just grabbing the text from their actual <label>s.

Let me know if there's a way to do this that's more in keeping with the repo

RobHannay · 2023-09-27T21:31:29Z

source/features/one-click-review-submission.tsx

-	for (const radio of radios) {
-		radio.closest('.form-checkbox')!.remove();
-	}
+	radios[0].closest('fieldset')!.remove();


The radios are now wrapped in a fieldset, which also has padding of its own, so it's cleaner just to remove.

Accessing the first radio and therefore its closest fieldset felt like the most straightforward way to achieve without having to find anything more specific about the fieldset (it only has an m-3 class)

capfei · 2023-09-27T22:41:14Z

Not sure if it is considered better or not but I was able to get the button text by replacing {radio.nextSibling} with {radio.labels[0].innerHTML}.

RobHannay · 2023-09-28T08:39:53Z

Not sure if it is considered better or not but I was able to get the button text by replacing {radio.nextSibling} with {radio.labels[0].innerHTML}.

Thanks @capfei, I like this suggestion

@fregante do you have a preference?

source/features/one-click-review-submission.tsx

fregante · 2023-09-28T08:55:05Z

It's safer to deal with nodes, innerHTML triggers extra checks by Mozilla https://stackoverflow.com/q/45579400/288906

Something you could optimize for would be to maintain compatibility with the older version so that we don't break GitHub Enterprise users

RobHannay · 2023-09-28T08:58:35Z

It's safer to deal with nodes, innerHTML triggers extra checks by Mozilla stackoverflow.com/q/45579400/288906

This is useful, thanks. I still kept textContent instead of innerHTML - does that mitigate the above?

Something you could optimize for would be to maintain compatibility with the older version so that we don't break GitHub Enterprise users

Ah yes, what's the usual process with this? When features get yolo-disabled I assume that also disables for GHE users?

fregante · 2023-09-28T09:08:39Z

This is useful, thanks. I still kept textContent instead of innerHTML - does that mitigate the above?

textContent is perfectly fine 👍

GitHub Enterprise users

Ah yes, what's the usual process with this?

It depends. For less visible features we just make the changes and wait for GHE users to open issues or submit patches, if they're necessary. See #6633

This feature is rather visible and quite important, so we can't risk breaking GHE users like #6963.

For GitHub changes that are introduced via Feature Preview, it's easy to toggle back and forth and see both DOM versions. In this case:

we write code that can work in both cases (e.g. keep the form attribute), OR
we use detection (e.g. if this element exist, OLD CODE, otherwise NEW CODE), keeping the changes to a minimum, OR
we keep a whole new copy of the code in cases where that detection makes the code too messy. See #6927

I suggest trying the first one, then we can tag a GHE user to test it out.

fregante · 2023-09-28T09:09:29Z

When features get yolo-disabled I assume that also disables for GHE users?

Yes. That also means that if we ship a feature that breaks GHE, we have to yolo-disable it on GitHub.com too

fregante · 2023-09-28T09:12:34Z

@busches can you paste the popup’s prettified HTML here so that we can work on a compatible fix?

RobHannay · 2023-09-28T10:05:12Z

Cool, thanks. I've merged the two version together to take a hybrid approach, where we always try the 'new' way but fallback to the old way if it's nullish.

Will await either the old DOM or someone to test it :)

busches · 2023-09-28T13:11:24Z

Here's what this PR looks like on GHE 3.9.4:

Here's the HTML:
modal.txt

RobHannay · 2023-09-28T13:12:46Z

Here's what this PR looks like on GHE 3.9.4:

Here's the HTML: modal.txt

Thanks @busches. On it

RobHannay · 2023-09-28T13:31:50Z

source/features/one-click-review-submission.tsx

-		const tooltip = radio.parentElement!.getAttribute('aria-label');
+		const labelNode = radio.labels?.[0];
+
+		const tooltip = labelNode?.nextElementSibling?.textContent;


So, weirdly it looks from the previous DOM structure like there wasn't an aria-label on its parent anyway. I suspect that could have broken a while ago. Not sure exactly what the intended behaviour was before, but I've actually re-removed the hybrid approach because I don't believe it was doing anything

There are many versions of GHE and the DOM changes quite a bit. I'm thankfully on a more recent version, but 3.7-3.10 are currently supported by GH themselves.

This label is grabbing too much on GHE:

<label class="d-block"> <input type="radio" name="pull_request_review[event]" value="comment" checked="" /> Comment <p class="text-normal color-fg-muted"> Submit general feedback without explicit approval. </p> </label>

Can do something "fun" like:

const label = labelNode?.nextElementSibling; const nestedDescription = label.getElementsByTaName('p'); // GHE has a nested P with description inside the label if (nestedDescription.length) { label.removeChild(nestedDescription[0]); } const tooltip = label.textContent;

I'm sure there's a better way to achieve that, but that should get us there.

RobHannay · 2023-09-28T13:33:23Z

source/features/one-click-review-submission.tsx

 				form={formAttribute}
 				value={radio.value}
 				className={classes.join(' ')}
 				aria-label={tooltip!}
 				disabled={radio.disabled}
 			>
-				{radio.nextSibling}
+				{/* Old || new. Backwards-compat label content queries. Issue #6963. */}
+				{radio.nextSibling?.textContent?.trim() || labelNode?.textContent}


So this label fix works by:

Trying to use the old approach first,

Failing properly if nextSibling doesn't contain any text (in the new structure, it was containing a line break hence the trim)

Falling back to the new way of doing labelling which only has the single-word labels, where the description is now somewhere else

RobHannay · 2023-09-28T13:45:44Z

I've hit two conflicting lint rules.

I want to use a logic OR for the label fallback; I can't use nullish coalescing operator because the "old" label will be an empty string if it doesn't exist. So I changed to use a ternary op a ? a : b which tbf is called out, and is worse imo.

Should I lint-ignore the nullish operator one or what? Is that only because it doesn't have enough type information?

fregante · 2023-09-28T10:17:18Z

source/features/one-click-review-submission.tsx

 	// Generate the new buttons
 	for (const radio of radios) {
-		const tooltip = radio.parentElement!.getAttribute('aria-label');
+		const labelNode = radio.labels?.[0];


Suggested change

const labelNode = radio.labels?.[0];

const [label] = radio.labels!;

The rule is to only use ?. if we expect the element to be missing in some cases. If we know it won't be missing, we use !

This applies to your other code too

fregante · 2023-09-28T14:59:11Z

source/features/one-click-review-submission.tsx

@@ -30,9 +29,13 @@ function replaceCheckboxes(originalSubmitButton: HTMLButtonElement): void {
 		);
 	}

+	const formAttribute = originalSubmitButton.getAttribute('form')!;


Let's move this back to line 12 to keep the blame history

fregante · 2023-10-02T12:34:53Z

I hope to see this completed soon so I can merge it ☺️ if you see any awkward lint errors you can leave them broken and I'll take care of them

fregante · 2023-10-04T11:00:57Z

@busches could you help bring this over to the finish line so I can publish it without having to disable the feature on GHE?

busches · 2023-10-05T01:28:11Z

@fregante I probably won't have time till Friday to take a look. If you have something to test, I get to that tomorrow.

fregante · 2023-10-05T05:44:13Z

All good now. Testing for the GHE version was as easy as pasting that HTML replacing the original <form>, and the selector-observer automatically reapplied the changes. Thanks @busches!

I also made a change that was long overdue: I dropped the green background from the "comment" button. It was technically correct (green = default action, on GitHub) but it was confusing (green = approval)

RobHannay · 2023-10-05T12:48:46Z

Thanks for getting it over the line 🙌

fix: one-click-review-submission to reflect GitHub's restructure of t…

cc6f027

…hat modal

RobHannay force-pushed the fix/one-click-pr-submission branch from 0ed7f92 to cc6f027 Compare September 27, 2023 21:26

RobHannay commented Sep 27, 2023

View reviewed changes

RobHannay marked this pull request as ready for review September 27, 2023 21:32

chore: remove now-redundant comment about form attribute

e89b174

a11y: carry descriptions too into tooltips

bc917a4

RobHannay commented Sep 28, 2023

View reviewed changes

source/features/one-click-review-submission.tsx Outdated Show resolved Hide resolved

fregante added the bug label Sep 28, 2023

RobHannay added 2 commits September 28, 2023 10:59

fix: add hybrid support for previous form structure

458a7bc

chore: capitalised comments

29ebc12

chore: revise backwards-compatibility using DOM reference

91a96fb

RobHannay commented Sep 28, 2023

View reviewed changes

chore: resolve nullish-coalescing lint error

0458486

fregante reviewed Sep 28, 2023

View reviewed changes

fregante self-assigned this Oct 5, 2023

Safer replacements

db88aa6

fregante changed the title ~~fix: one-click-review-submission to reflect GitHub's restructure of that modal~~ Restore one-click-review-submission feature Oct 5, 2023

fregante merged commit 5247e6e into refined-github:main Oct 5, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore `one-click-review-submission` feature #6964

Restore `one-click-review-submission` feature #6964

RobHannay commented Sep 27, 2023 •

edited

RobHannay Sep 27, 2023

RobHannay Sep 27, 2023

capfei commented Sep 27, 2023

RobHannay commented Sep 28, 2023

fregante commented Sep 28, 2023 •

edited

RobHannay commented Sep 28, 2023

fregante commented Sep 28, 2023 •

edited

fregante commented Sep 28, 2023

fregante commented Sep 28, 2023 •

edited

RobHannay commented Sep 28, 2023

busches commented Sep 28, 2023 •

edited

RobHannay commented Sep 28, 2023

RobHannay Sep 28, 2023

busches Sep 28, 2023

busches Oct 5, 2023

RobHannay Sep 28, 2023

RobHannay commented Sep 28, 2023

fregante Sep 28, 2023

fregante Sep 28, 2023 •

edited

fregante commented Oct 2, 2023

fregante commented Oct 4, 2023

busches commented Oct 5, 2023

fregante commented Oct 5, 2023

RobHannay commented Oct 5, 2023

	const labelNode = radio.labels?.[0];
	const [label] = radio.labels!;

Restore one-click-review-submission feature #6964

Restore one-click-review-submission feature #6964

Conversation

RobHannay commented Sep 27, 2023 • edited

Test URLs

Screenshot

Description tooltips

RobHannay Sep 27, 2023

Choose a reason for hiding this comment

RobHannay Sep 27, 2023

Choose a reason for hiding this comment

capfei commented Sep 27, 2023

RobHannay commented Sep 28, 2023

fregante commented Sep 28, 2023 • edited

RobHannay commented Sep 28, 2023

fregante commented Sep 28, 2023 • edited

fregante commented Sep 28, 2023

fregante commented Sep 28, 2023 • edited

RobHannay commented Sep 28, 2023

busches commented Sep 28, 2023 • edited

RobHannay commented Sep 28, 2023

RobHannay Sep 28, 2023

Choose a reason for hiding this comment

busches Sep 28, 2023

Choose a reason for hiding this comment

busches Oct 5, 2023

Choose a reason for hiding this comment

RobHannay Sep 28, 2023

Choose a reason for hiding this comment

RobHannay commented Sep 28, 2023

fregante Sep 28, 2023

Choose a reason for hiding this comment

fregante Sep 28, 2023 • edited

Choose a reason for hiding this comment

fregante commented Oct 2, 2023

fregante commented Oct 4, 2023

busches commented Oct 5, 2023

fregante commented Oct 5, 2023

RobHannay commented Oct 5, 2023

Restore `one-click-review-submission` feature #6964

Restore `one-click-review-submission` feature #6964

RobHannay commented Sep 27, 2023 •

edited

fregante commented Sep 28, 2023 •

edited

fregante commented Sep 28, 2023 •

edited

fregante commented Sep 28, 2023 •

edited

busches commented Sep 28, 2023 •

edited

fregante Sep 28, 2023 •

edited