Skip to content

fix(ui): red OK button on destructive confirm popovers (#314) #814

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.

Sign up for GitHub

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

Open
skearnes wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ui): red OK button on destructive confirm popovers (#314) #814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ui/src/common/components/interactions/ConfirmPopover/ConfirmPopover.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,32 @@ describe('ConfirmPopover', () => {
fireEvent.click(screen.getByRole('button', { name: 'Cancel' }));
expect(onCancel).toHaveBeenCalledTimes(1);
});

const renderPopover = (destructive?: boolean) =>
renderWithMantine(
<ConfirmPopover
opened
destructive={destructive}
target={<button>open</button>}
title="Remove?"
text="x"
onConfirm={vi.fn()}
onCancel={vi.fn()}
/>,
);

it('renders a red OK button for destructive confirmations (#314)', () => {
renderPopover(true);
// Mantine color="red" inlines the red color custom properties on the button element.
expect(
screen.getByRole('button', { name: 'OK' }).getAttribute('style') ?? '',
).toContain('red');
});

it('renders a default (non-red) OK button when not destructive', () => {
renderPopover(false);
expect(
screen.getByRole('button', { name: 'OK' }).getAttribute('style') ?? '',
).not.toContain('red');
Comment on lines +57 to +69

@greptile-apps greptile-apps Bot Jun 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Style-attribute sniffing couples tests to Mantine internals

Both new tests assert on the raw style attribute string to detect whether the button is red, relying on the fact that Mantine currently serialises color="red" as inline CSS custom properties that happen to contain the string "red". If Mantine ever moves colour application to a CSS class or changes the custom-property naming convention, both tests would pass or fail spuriously without any change to the component itself. A more robust assertion would check the Mantine data-* attribute or the computed class name that signals the colour variant — or simply verify that the color prop is forwarded correctly at the component level. This is non-blocking, but worth considering before the test pattern is copied elsewhere.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

});
});
4 changes: 4 additions & 0 deletions ui/src/common/components/interactions/ConfirmPopover/ConfirmPopover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ interface ConfirmPopoverProps extends PopoverProps {
text: string;
onConfirm: () => void;
onCancel: () => void;
// Render the confirm (OK) button in red for destructive actions (remove/delete/unshare). (#314)
destructive?: boolean;
}

export function ConfirmPopover({
Expand All @@ -31,6 +33,7 @@ export function ConfirmPopover({
text,
onConfirm,
onCancel,
destructive,
...rest
}: Readonly<ConfirmPopoverProps>) {
return (
Expand Down Expand Up @@ -79,6 +82,7 @@ export function ConfirmPopover({
<Button
className={classes.popoverButton}
size="xs"
color={destructive ? 'red' : undefined}
onClick={onConfirm}
>
OK
Expand Down
1 change: 1 addition & 0 deletions ui/src/features/datasets/DatasetHeader/DatasetHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export function DatasetHeader({ dataset }: Readonly<DatasetHeaderProps>) {
>
{canDatasetBeDeleted && (
<ConfirmPopover
destructive
opened={removeConfirmOpened}
position="right"
offset={8}
Expand Down
1 change: 1 addition & 0 deletions ui/src/features/datasets/ShareDataset/ShareDatasetSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ function GroupListItem({ group, onUnshareWithGroup }: Readonly<GroupsListItemPro
/>
</Flex>
<ConfirmPopover
destructive
opened={popoverOpened}
onCancel={close}
onConfirm={() => onUnshareWithGroup(group.id)}
Expand Down
1 change: 1 addition & 0 deletions ui/src/features/reactions/ReactionEntities/ReactionEntityDelete/ReactionEntityDelete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export function ReactionEntityDelete({

return (
<ConfirmPopover
destructive
title={`Remove ${entityName}`}
text={`Are you sure to remove this ${entityName}?`}
opened={confirmationOpened}
Expand Down
1 change: 1 addition & 0 deletions ui/src/features/reactions/RemoveReaction/RemoveReaction.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export function RemoveReaction({ reactionId }: Readonly<RemoveReactionProps>) {

return (
<ConfirmPopover
destructive
title={`Remove this ${entityToRemove}`}
text={`Are you sure you want to remove this ${entityToRemove}?`}
opened={confirmationOpened}
Expand Down
Loading