Match dialog button layout to OS #6044
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (C) 2025 Posit Software, PBC. All rights reserved. | ||
| * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
|
|
||
| // React. | ||
| import React from 'react'; | ||
|
|
||
| // Other dependencies. | ||
| import * as platform from '../../../../../base/common/platform.js'; | ||
|
|
||
| /** | ||
| * PlatformNativeDialogActionBarProps interface. | ||
| */ | ||
| interface PlatformNativeDialogActionBarProps { | ||
| secondaryButton?: React.ReactNode, | ||
| primaryButton?: React.ReactNode | ||
| } | ||
|
|
||
| /** | ||
| * PlatformNativeDialogActionBar component. | ||
| * @param props A PlatformNativeDialogActionBarProps that contains the component properties. | ||
| * @returns The rendered component. | ||
| */ | ||
| export const PlatformNativeDialogActionBar = ({ secondaryButton, primaryButton }: PlatformNativeDialogActionBarProps) => { | ||
|
There was a problem hiding this comment. This is a super basic component that renders the primary and secondary button in the order most common for a given OS. Windows always positions the cancel/secondary button after the primary button and the inverse is true for Mac. |
||
| // Render. | ||
| return ( | ||
| <> | ||
| { | ||
| platform.isWindows | ||
| ? <>{primaryButton}{secondaryButton}</> | ||
| : <>{secondaryButton}{primaryButton}</> | ||
| } | ||
| </> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| /*--------------------------------------------------------------------------------------------- | ||
| * Copyright (C) 2024 Posit Software, PBC. All rights reserved. | ||
| * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. | ||
| *--------------------------------------------------------------------------------------------*/ | ||
|
|
||
| // CSS. | ||
| import './positronModalDialog.css'; | ||
| import './confirmDeleteModalDialog.css'; | ||
|
|
||
| // React. | ||
| import React, { PropsWithChildren } from 'react'; | ||
|
|
||
| // Other dependencies. | ||
| import { localize } from '../../../../nls.js'; | ||
| import { Button } from '../../../../base/browser/ui/positronComponents/button/button.js'; | ||
| import { ContentArea } from './components/contentArea.js'; | ||
| import { PositronModalDialog, PositronModalDialogProps } from './positronModalDialog.js'; | ||
| import { PlatformNativeDialogActionBar } from './components/platformNativeDialogActionBar.js'; | ||
|
|
||
| /** | ||
| * ConfirmDeleteModalDialogProps interface. | ||
| */ | ||
| export interface ConfirmDeleteModalDialogProps extends PositronModalDialogProps { | ||
|
There was a problem hiding this comment. This modal has been simplified in its usage so its shown when we want a user to see a confirmation modal for a delete action. We always provide the delete action now and optionally can provide a different label for the delete action. The |
||
| title: string; | ||
| cancelButtonTitle?: string; | ||
| deleteActionTitle?: string | ||
| onCancel: () => (void | Promise<void>); | ||
| onDeleteAction: () => (void | Promise<void>); | ||
| } | ||
|
|
||
| /** | ||
| * ConfirmDeleteModalDialog component. | ||
| * @param props A PropsWithChildren<ConfirmDeleteModalDialogProps> that contains the component | ||
| * properties. | ||
| * @returns The rendered component. | ||
| */ | ||
| export const ConfirmDeleteModalDialog = (props: PropsWithChildren<ConfirmDeleteModalDialogProps>) => { | ||
| const cancelButton = ( | ||
| <Button | ||
| className='action-bar-button' | ||
| onPressed={async () => await props.onCancel()} | ||
| > | ||
| {props.cancelButtonTitle ?? localize('positron.cancel', "Cancel")} | ||
| </Button> | ||
| ); | ||
| const deleteButton = ( | ||
| <Button | ||
| className='action-bar-button default' | ||
| onPressed={async () => await props.onDeleteAction()} | ||
| > | ||
| {props.deleteActionTitle ?? localize('positron.delete', "Delete")} | ||
| </Button> | ||
| ); | ||
|
|
||
| // Render. | ||
| return ( | ||
| <PositronModalDialog {...props}> | ||
| <ContentArea> | ||
| {props.children} | ||
| </ContentArea> | ||
| <div className='action-bar top-separator'> | ||
| <div className='left-actions'> | ||
| </div> | ||
| <div className='right-actions'> | ||
|
There was a problem hiding this comment. It looks like we explicitly want the button order here to be delete/cancel. This button order may seem odd to non-windows users now that we are trying to match native OS button ordering but I have left it as is for now because it looked like the button order was intentionally made so delete was the secondary action. I think we should change it to be native to the OS but would love to get thoughts on this from others. There was a problem hiding this comment. cc @juliasilge There was a problem hiding this comment. +1 to OS-applicable button ordering! There was a problem hiding this comment. With all this work to make our modals consistent with OS norms, it seems a shame to not really be thorough and go the whole way. I know there was some concern about it being too easy to take a destructive action, but it's still multiple user actions to actually delete, for example, the variables content. I would advocate for OS-consistent button ordering. |
||
| <PlatformNativeDialogActionBar secondaryButton={cancelButton} primaryButton={deleteButton} /> | ||
| </div> | ||
| </div> | ||
| </PositronModalDialog> | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the
ok-cancel-action-barclass took-cancel-back-action-barto avoid name collisions. The layout is different for this action bar and theOkCancelActionBar:justify-content: space-betweenvsjustify-content: end.This change prevents the
OKCancelModalDialogusing theOkCancelActionBarfrom having the buttons positioned incorrectly. I noticed during my testing that the styles from this sheet were overriding the styles inOkCancelActionBarwhich we don't want anymore.Here's a screenshot of what was happening:
