Match dialog button layout to OS #6044
Conversation
Order buttons in dialogs using the order preferred on a platform. Default to non-windows button order when platform is indeterminable.
Some modals using the okCancelActionBar were getting the css styles for the okCancelBackNextActionBar that caused button positions to be incorrect. This avoids class style ambiguity from being applied.
|
E2E Tests 🚀 |
| * @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.
| import { Button } from '../../../../base/browser/ui/positronComponents/button/button.js'; | ||
| import { ContentArea } from './components/contentArea.js'; | ||
| import { PositronModalDialog, PositronModalDialogProps } from './positronModalDialog.js'; | ||
|
|
||
| /** | ||
| * ConfirmationModalDialogProps interface. |
There was a problem hiding this comment.
This component was renamed to confirmDeleteModalDialog to make it clear when to use this dialog. It currently is on only being used to create the deleteAllVariablesModalDialog and the only difference between this dialog and the ok/cancel dialog looked to be the logic to handle styling for a destructive button.
If I misunderstood what we were trying to do with this dialog or think there's issues with this change, let me know and I'll undo it!
| @@ -45,22 +44,17 @@ export const ConfirmationModalDialog = (props: PropsWithChildren<ConfirmationMod | |||
| <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
| */ | ||
| export interface ConfirmationModalDialogProps extends PositronModalDialogProps { | ||
| 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 secondaryActionDestructive boolean felt unnecessary to me because the OKCancelModalDialog is used when we need a basic confirmation dialog for non-destructive actions. This dialog felt like it was trying to do both even though we have a separate dialog we are using for confirming non destructive actions.
| @@ -3,8 +3,7 @@ | |||
| * Licensed under the Elastic License 2.0. See LICENSE.txt for license information. | |||
| *--------------------------------------------------------------------------------------------*/ | |||
|
|
|||
| .positron-modal-dialog-box | |||
| .ok-cancel-action-bar { | |||
| .positron-modal-dialog-box .ok-cancel-back-action-bar { | |||
There was a problem hiding this comment.
I renamed the ok-cancel-action-bar class to ok-cancel-back-action-bar to avoid name collisions. The layout is different for this action bar and the OkCancelActionBar: justify-content: space-between vs justify-content: end.
This change prevents the OKCancelModalDialog using the OkCancelActionBar from having the buttons positioned incorrectly. I noticed during my testing that the styles from this sheet were overriding the styles in OkCancelActionBar which we don't want anymore.
Here's a screenshot of what was happening:
| <div className='left-actions'> | ||
| {backButtonConfig ? | ||
| <Button className='button action-bar-button' onPressed={backButtonConfig.onClick} disabled={backButtonConfig.disable ?? false}> |
There was a problem hiding this comment.
I noticed that we had this button class added to a bunch of buttons in the modals but I couldn't find the class in any of the relevant stylesheets. I think this may be an orphaned class that was left behind by accident? I didn't see any breakage after I removed it.
Let me know if this is wrong and we do need it!
src/vs/workbench/contrib/positronPlots/browser/modalDialogs/setPlotSizeModalDialog.tsx
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/positronPlots/browser/modalDialogs/setPlotSizeModalDialog.tsx
Outdated
Show resolved
Hide resolved
This change updates the order of buttons in modals to match a user's native OS layout. The layout of buttons defaults to OS X button order when Positron is run in a web environment.
Addresses #3548
Release Notes
New Features
Bug Fixes
QA Notes
Screenshots
Screen.Recording.2025-01-17.at.12.19.49.PM.mov
@:new-project-wizard @:win