Skip to content

Commit 8a8a4db

Browse files
Copilotalexr00
andauthored
Make Comment the primary action in review dropdown (#8139)
* Initial plan * Make Comment the primary action in review dropdown with proper ordering and styling Co-authored-by: alexr00 <[email protected]> * clean up --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: alexr00 <[email protected]>
1 parent 0e0f673 commit 8a8a4db

File tree

3 files changed

+29
-18
lines changed

3 files changed

+29
-18
lines changed

package.json

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3480,16 +3480,19 @@
34803480
"group": "1_create@2"
34813481
},
34823482
{
3483-
"command": "review.approve",
3484-
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentApprove"
3483+
"command": "review.comment",
3484+
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentComment",
3485+
"group": "1_review@1"
34853486
},
34863487
{
3487-
"command": "review.comment",
3488-
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentComment"
3488+
"command": "review.approve",
3489+
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentApprove",
3490+
"group": "1_review@2"
34893491
},
34903492
{
34913493
"command": "review.requestChanges",
3492-
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentRequestChanges"
3494+
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentRequestChanges",
3495+
"group": "1_review@3"
34933496
},
34943497
{
34953498
"command": "review.approveOnDotCom",
@@ -3500,16 +3503,19 @@
35003503
"when": "webviewId == 'github:activePullRequest' && github:reviewCommentMenu && github:reviewCommentRequestChangesOnDotCom"
35013504
},
35023505
{
3503-
"command": "review.approveDescription",
3504-
"when": "webviewId == PullRequestOverview && github:reviewCommentMenu && github:reviewCommentApprove"
3506+
"command": "review.commentDescription",
3507+
"when": "webviewId == PullRequestOverview && github:reviewCommentMenu && github:reviewCommentComment",
3508+
"group": "1_review@1"
35053509
},
35063510
{
3507-
"command": "review.commentDescription",
3508-
"when": "webviewId == PullRequestOverview && github:reviewCommentMenu && github:reviewCommentComment"
3511+
"command": "review.approveDescription",
3512+
"when": "webviewId == PullRequestOverview && github:reviewCommentMenu && github:reviewCommentApprove",
3513+
"group": "1_review@2"
35093514
},
35103515
{
35113516
"command": "review.requestChangesDescription",
3512-
"when": "webviewId == PullRequestOverview && github:reviewCommentMenu && github:reviewCommentRequestChanges"
3517+
"when": "webviewId == PullRequestOverview && github:reviewCommentMenu && github:reviewCommentRequestChanges",
3518+
"group": "1_review@3"
35133519
},
35143520
{
35153521
"command": "review.approveOnDotComDescription",

webviews/components/comment.tsx

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -460,12 +460,12 @@ export function AddComment({
460460
defaultOptionValue={() => currentSelection}
461461
allOptions={() => {
462462
const actions: { label: string; value: string; optionDisabled: boolean; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[] = [];
463-
if (availableActions.approve) {
464-
actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton });
465-
}
466463
if (availableActions.comment) {
467464
actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment), optionDisabled: shouldDisableNonApproveButtons });
468465
}
466+
if (availableActions.approve) {
467+
actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton });
468+
}
469469
if (availableActions.requestChanges) {
470470
actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges), optionDisabled: shouldDisableNonApproveButtons });
471471
}
@@ -475,6 +475,7 @@ export function AddComment({
475475
disabled={isBusy || busy}
476476
hasSingleAction={Object.keys(availableActions).length === 1}
477477
spreadable={true}
478+
primaryOptionValue={ReviewType.Comment}
478479
/>
479480
</div>
480481
</form>
@@ -608,12 +609,12 @@ export const AddCommentSimple = (pr: PullRequest) => {
608609
defaultOptionValue={() => currentSelection}
609610
allOptions={() => {
610611
const actions: { label: string; value: string; optionDisabled: boolean; action: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void }[] = [];
611-
if (availableActions.approve) {
612-
actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton });
613-
}
614612
if (availableActions.comment) {
615613
actions.push({ label: availableActions[ReviewType.Comment]!, value: ReviewType.Comment, action: () => submitAction(ReviewType.Comment), optionDisabled: shouldDisableNonApproveButtons });
616614
}
615+
if (availableActions.approve) {
616+
actions.push({ label: availableActions[ReviewType.Approve]!, value: ReviewType.Approve, action: () => submitAction(ReviewType.Approve), optionDisabled: shouldDisableApproveButton });
617+
}
617618
if (availableActions.requestChanges) {
618619
actions.push({ label: availableActions[ReviewType.RequestChanges]!, value: ReviewType.RequestChanges, action: () => submitAction(ReviewType.RequestChanges), optionDisabled: shouldDisableNonApproveButtons });
619620
}
@@ -623,6 +624,7 @@ export const AddCommentSimple = (pr: PullRequest) => {
623624
disabled={isBusy || pr.busy}
624625
hasSingleAction={Object.keys(availableActions).length === 1}
625626
spreadable={true}
627+
primaryOptionValue={ReviewType.Comment}
626628
/>
627629
</div>
628630
</span>

webviews/components/contextDropdown.tsx

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ interface ContextDropdownProps {
1717
hasSingleAction?: boolean;
1818
spreadable: boolean;
1919
isSecondary?: boolean;
20+
primaryOptionValue?: string;
2021
}
2122

2223
function useWindowSize() {
@@ -32,7 +33,7 @@ function useWindowSize() {
3233
return size;
3334
}
3435

35-
export const ContextDropdown = ({ optionsContext, defaultOptionLabel, defaultOptionValue, defaultAction, allOptions: options, optionsTitle, disabled, hasSingleAction, spreadable, isSecondary }: ContextDropdownProps) => {
36+
export const ContextDropdown = ({ optionsContext, defaultOptionLabel, defaultOptionValue, defaultAction, allOptions: options, optionsTitle, disabled, hasSingleAction, spreadable, isSecondary, primaryOptionValue }: ContextDropdownProps) => {
3637
const [expanded, setExpanded] = useState(false);
3738
const onHideAction = (e: MouseEvent | KeyboardEvent) => {
3839
if (e.target instanceof HTMLElement && e.target.classList.contains('split-right')) {
@@ -56,7 +57,9 @@ export const ContextDropdown = ({ optionsContext, defaultOptionLabel, defaultOpt
5657

5758
return <div className={`dropdown-container${spreadable ? ' spreadable' : ''}`} ref={divRef}>
5859
{divRef.current && spreadable && (divRef.current.clientWidth > 375) && options && !hasSingleAction ? options().map(({ label, value, action, optionDisabled }) => {
59-
return <button className='inlined-dropdown' key={value} title={label} disabled={optionDisabled || disabled} onClick={action} value={value}>{label}</button>;
60+
// Only the primary option should use the primary (blue) button style when expanded
61+
const isPrimary = primaryOptionValue && value === primaryOptionValue;
62+
return <button className={`inlined-dropdown${isPrimary ? '' : ' secondary'}`} key={value} title={label} disabled={optionDisabled || disabled} onClick={action} value={value}>{label}</button>;
6063
})
6164
:
6265
<div className='primary-split-button'>

0 commit comments

Comments
 (0)