fix(ui): red OK button on destructive confirm popovers (#314) - #814
fix(ui): red OK button on destructive confirm popovers (#314)#814skearnes wants to merge 1 commit into
Conversation
The "Remove" buttons turn red on hover (#776); make the confirmation match. Add an optional `destructive` prop to ConfirmPopover that renders the OK button in red, and apply it to the remove/delete/unshare confirmations (RemoveReaction, ReactionEntityDelete, dataset remove in DatasetHeader, dataset unshare in ShareDatasetSidebar). Non-destructive confirmations are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 Pulumi Neo didn't review this pull request: no Pulumi preview ran for it. Agentic reviews require a preview of the affected stacks (for example from your CI's |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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'); |
There was a problem hiding this comment.
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!
1 participant
Closes #314.
The destructive Remove buttons already turn red on hover (#776); this makes the confirmation popover match. Adds an optional
destructiveprop toConfirmPopoverthat renders the OK button in red, applied to the remove/delete/unshare confirmations:RemoveReaction,ReactionEntityDelete(reaction + entity remove)DatasetHeader(remove dataset)ShareDatasetSidebar(unshare dataset from a group)Non-destructive confirmations (enumeration cancel, role change, share) are unchanged. Added unit tests asserting the OK button is red with
destructiveand default without.tsc -b+ lint + affected component tests green.🤖 Generated with Claude Code
Greptile Summary
Adds an optional
destructiveboolean prop toConfirmPopoverthat renders the OK button in red (color="red") for irreversible actions, and applies it to all four remove/delete/unshare call-sites. Non-destructive confirmations are untouched.ConfirmPopoverreceivesdestructive?in its props interface, destructures it before...rest, and passescolor={destructive ? 'red' : undefined}to the MantineButton— clean and minimal."red", which works today but could drift with a Mantine version bump.DatasetHeader,ShareDatasetSidebar,ReactionEntityDelete,RemoveReaction) are updated consistently.Confidence Score: 5/5
Safe to merge — the change is a small additive UI prop with no logic or data-flow implications.
All four destructive call-sites are updated consistently, the new prop is fully optional so existing non-destructive uses need no changes, and the implementation is a single ternary. The only note is that the new tests sniff a raw style-attribute string, which could drift with a Mantine upgrade, but that is a test-maintenance concern rather than a functional defect.
No files require special attention, though the style-attribute assertions in
ConfirmPopover.test.tsxare worth keeping in mind if Mantine is upgraded.Important Files Changed
destructiveprop; passescolor="red"to the OK Button when truthy. Clean prop destructuring before...restspread.styleattribute for "red", which works but couples the test to Mantine's CSS-custom-property injection — could break silently on a Mantine upgrade.destructiveto the remove-datasetConfirmPopover; no other changes.destructiveto the unshare-groupConfirmPopover; no other changes.destructiveto the entity-deleteConfirmPopover; no other changes.destructiveto the remove-reactionConfirmPopover; no other changes.Reviews (1): Last reviewed commit: "fix(ui): red OK button on destructive co..." | Re-trigger Greptile