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

Restore PR review buttons on GitHub Enterprise #6635

Merged
merged 9 commits into from
May 9, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 18 additions & 20 deletions source/features/one-click-review-submission.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@ import * as pageDetect from 'github-url-detection';
import {CheckIcon, FileDiffIcon} from '@primer/octicons-react';

import features from '../feature-manager.js';
import observe from '../helpers/selector-observer.js';

function addButtons(radios: HTMLInputElement[]): void {
const form = radios[0].form!;
const actionsRow
= select('.form-actions', form) // TODO: Drop after September 2023
?? form.closest('.SelectMenu')!.querySelector('.form-actions')!;
function replaceCheckboxes(originalSubmitButton: HTMLButtonElement): void {
const form = originalSubmitButton.form!;
const actionsRow = originalSubmitButton.closest('.form-actions')!;
const formAttribute = originalSubmitButton.getAttribute('form')!;
Comment on lines +9 to +12
Copy link
Member Author

Choose a reason for hiding this comment

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

By targeting the original submit button, it's easier to extrapolate all the necessary elements without conditions


// Do not use `select.all` because elements can be outside `form`
// `RadioNodeList` is dynamic, so we need to make a copy
const radios = [...form.elements.namedItem('pull_request_review[event]') as RadioNodeList] as HTMLInputElement[];
Copy link
Member Author

Choose a reason for hiding this comment

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


// Set the default action for cmd+enter to Comment
if (radios.length > 1) {
Expand Down Expand Up @@ -41,7 +45,7 @@ function addButtons(radios: HTMLInputElement[]): void {
type="submit"
name="pull_request_review[event]"
// The buttons are no longer inside the form itself; this links the form
form={form.contains(actionsRow) ? undefined : form.id} // TODO: Drop condition after September 2023
form={formAttribute}
value={radio.value}
className={classes.join(' ')}
aria-label={tooltip!}
Expand All @@ -65,8 +69,7 @@ function addButtons(radios: HTMLInputElement[]): void {
radio.closest('.form-checkbox')!.remove();
}

// The selector excludes the "Cancel" button
select('[type="submit"]:not([name])', actionsRow)!.remove();
originalSubmitButton.remove();
}

let lastSubmission: number | undefined;
Expand All @@ -80,22 +83,15 @@ function blockDuplicateSubmissions(event: DelegateEvent): void {
lastSubmission = Date.now();
}

function init(signal: AbortSignal): false | void {
// Freeze form to avoid duplicate submissions
delegate('[action$="/reviews"]', 'submit', blockDuplicateSubmissions, {signal});

// `return false` must always be after delegated events are added
const radios = select.all('input[type="radio"][name="pull_request_review[event]"]');
if (radios.length === 0) {
return false;
}

addButtons(radios);
function init(signal: AbortSignal): void {
// The selector excludes the "Cancel" button
observe('#review-changes-modal [type="submit"]:not([name])', replaceCheckboxes, {signal});
delegate('#review-changes-modal form', 'submit', blockDuplicateSubmissions, {signal});
}

void features.add(import.meta.url, {
include: [
pageDetect.isPR,
pageDetect.isPRFiles,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why this was so broad

Copy link
Member

Choose a reason for hiding this comment

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

The review option shows on a specific commit of a PR, that no longer works now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed here: added isPRCommit as done everywhere else in the extension.

],
awaitDomReady: true,
init,
Expand All @@ -109,3 +105,5 @@ https://github.com/refined-github/sandbox/pull/4/files
https://github.com/refined-github/sandbox/pull/12/files

*/

console.log(1);
fregante marked this conversation as resolved.
Show resolved Hide resolved