-
Notifications
You must be signed in to change notification settings - Fork 8
PLU-361: warning when deleting connections #794
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.
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
Conversation
| header: `Duplicate ${dialogHeader}`, | ||
| body: `You'll need to replace the data in every step and test each step in your duplicated ${dialogHeader?.toLowerCase()} before publishing it.`, |
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.
actually the description will be quite unique to duplicate pipe only right? maybe it should not need the dialog header variable here
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.
initially thought maybe parameterise in case there's a duplicate tile function in future, but agree we can change to var when we need next time
| import { Button } from '@opengovsg/design-system-react' | ||
|
|
||
| export type AlertDialogType = 'delete' | 'duplicate' | ||
| export type AlertHeaderType = 'App' | 'Connection' | 'Pipe' | 'Tile' |
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.
just checking is app being used anywhere?
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.
you're right, this is not being used by App
|
|
||
| function AppConnectionRow(props: AppConnectionRowProps): React.ReactElement { | ||
| const toast = useToast() | ||
| const [dialogType, setDialogType] = useState<AlertDialogType>('delete') // delete by default |
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 think you won't need this since there's only one delete action possible in this file right?
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.
good point, unlikely to have other actions any time soon, will remove
m0nggh
left a comment
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.
nice refactor, just have some nits but tested that functionality still works!
Datadog ReportBranch report: ✅ 0 Failed, 687 Passed, 0 Skipped, 1m 56.37s Total Time 🔻 Code Coverage Decreases vs Default Branch (1)
|
m0nggh
left a comment
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.
ok nice lgtm
Problem
There is no warning/prompt when user is deleting a connection.
Other delete actions on Apps/Pipes/Tiles have a warning/prompt when user is attempting to delete.
Closes PLU-361: warning-when-deleting-connections
Solution
Add warning/prompt modal when user is attempting to delete a connection.
Improvements:
Before & After Screenshots
BEFORE:
Screen.Recording.2024-11-11.at.1.45.57.PM.mov
AFTER:
Screen.Recording.2024-11-11.at.1.47.42.PM.mov
Screen.Recording.2024-11-11.at.2.44.37.PM.mov
Tests
New scripts:
packages/frontend/src/components/MenuAlertDialog/index.tsx: generic MenuAlertDialog for reuse across delete actions (updated fromplumber/packages/frontend/src/components/FlowRow/MenuAlertDialog.tsx)