mirror of
https://github.com/kuhyx/todo-app.git
synced 2026-11-10 19:07:02 +01:00
Confirm before deleting a note
Both delete paths -- the per-note actions sheet and the detail screen -- deleted immediately, with no confirmation and no undo. With a pointer that is merely risky: you have to travel to the control and press it. With a keyboard it is one Tab away from a Return you were about to press anyway, and the note is gone with nothing to recover it. The confirmation is built so the SAFE option is what a reflexive keypress hits: Cancel is autofocused, Escape dismisses as "no", and the destructive button is error-coloured but deliberately not the default action. Escape is bound explicitly rather than relying on the barrier, because the desktop surface here is a Chrome --app window where that cannot be assumed -- and a confirmation the keyboard cannot back out of is worse than no confirmation at all. The actions sheet also gains isScrollControlled. Default bottom sheets cap at 9/16 of the viewport, i.e. ~432px on a 768px screen, and this sheet's height grows with the note title and its chip rows -- so Delete could be clipped out of reach on the very screen this app is used on. The two existing delete tests correctly failed once confirmation landed; both now go through the dialog, and each gains a cancel-keeps-the-note counterpart so the negative path is covered too. 204 tests, 100% line coverage held. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
72c5c21d9a
commit
e103261bc8
84
lib/ui/confirm.dart
Normal file
84
lib/ui/confirm.dart
Normal file
@ -0,0 +1,84 @@
|
||||
/// Confirmation for destructive actions.
|
||||
library;
|
||||
|
||||
import 'package:flutter/material.dart';
|
||||
import 'package:flutter/services.dart';
|
||||
|
||||
/// Asks the user to confirm a destructive action, returning true if they do.
|
||||
///
|
||||
/// Exists because "delete note" had no confirmation and no undo on either
|
||||
/// path: the actions sheet and the detail screen both deleted immediately.
|
||||
/// With a pointer that is merely risky — you have to travel to the control and
|
||||
/// press it. With a keyboard it is one Tab away from a Return you were about
|
||||
/// to press anyway, and the note is gone with nothing to recover it.
|
||||
///
|
||||
/// Defaults are chosen so the *safe* option is what a reflexive keypress hits:
|
||||
/// Cancel is autofocused, and Escape dismisses. The destructive button is
|
||||
/// styled with the error color but is deliberately not the default action.
|
||||
///
|
||||
/// Returns false when dismissed by Escape, by the barrier, or by Cancel.
|
||||
Future<bool> confirmDestructive(
|
||||
BuildContext context, {
|
||||
required String title,
|
||||
required String message,
|
||||
String confirmLabel = 'Delete',
|
||||
String cancelLabel = 'Cancel',
|
||||
}) async {
|
||||
final confirmed = await showDialog<bool>(
|
||||
context: context,
|
||||
builder: (dialogContext) => _ConfirmDialog(
|
||||
title: title,
|
||||
message: message,
|
||||
confirmLabel: confirmLabel,
|
||||
cancelLabel: cancelLabel,
|
||||
),
|
||||
);
|
||||
return confirmed ?? false;
|
||||
}
|
||||
|
||||
class _ConfirmDialog extends StatelessWidget {
|
||||
const _ConfirmDialog({
|
||||
required this.title,
|
||||
required this.message,
|
||||
required this.confirmLabel,
|
||||
required this.cancelLabel,
|
||||
});
|
||||
|
||||
final String title;
|
||||
final String message;
|
||||
final String confirmLabel;
|
||||
final String cancelLabel;
|
||||
|
||||
@override
|
||||
Widget build(BuildContext context) {
|
||||
return CallbackShortcuts(
|
||||
// Escape dismisses as "no". Flutter's own barrier handles this on most
|
||||
// platforms, but the desktop surface here is a Chrome `--app` window
|
||||
// where that cannot be assumed, and a confirmation the keyboard cannot
|
||||
// back out of is worse than no confirmation at all.
|
||||
bindings: {
|
||||
const SingleActivator(LogicalKeyboardKey.escape): () =>
|
||||
Navigator.of(context).pop(false),
|
||||
},
|
||||
child: AlertDialog(
|
||||
title: Text(title),
|
||||
content: Text(message),
|
||||
actions: [
|
||||
TextButton(
|
||||
// Autofocused so the reflexive Return presses Cancel, not Delete.
|
||||
autofocus: true,
|
||||
onPressed: () => Navigator.of(context).pop(false),
|
||||
child: Text(cancelLabel),
|
||||
),
|
||||
TextButton(
|
||||
style: TextButton.styleFrom(
|
||||
foregroundColor: Theme.of(context).colorScheme.error,
|
||||
),
|
||||
onPressed: () => Navigator.of(context).pop(true),
|
||||
child: Text(confirmLabel),
|
||||
),
|
||||
],
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
@ -3,6 +3,7 @@ import 'package:flutter/material.dart';
|
||||
import 'package:todo/data/note.dart';
|
||||
import 'package:todo/data/note_repository.dart';
|
||||
import 'package:todo/data/note_template.dart';
|
||||
import 'package:todo/ui/confirm.dart';
|
||||
import 'package:todo/ui/note_editor.dart';
|
||||
|
||||
/// Full-screen view of a single note: read it in full, edit its body through
|
||||
@ -45,6 +46,12 @@ class _NoteDetailScreenState extends State<NoteDetailScreen> {
|
||||
_persist(_note.copyWith(text: text, updatedAt: DateTime.now()));
|
||||
|
||||
Future<void> _delete() async {
|
||||
final confirmed = await confirmDestructive(
|
||||
context,
|
||||
title: 'Delete note?',
|
||||
message: 'This cannot be undone.',
|
||||
);
|
||||
if (!confirmed) return;
|
||||
await widget.repository.delete(_note.id);
|
||||
if (mounted) Navigator.of(context).pop();
|
||||
}
|
||||
|
||||
@ -5,6 +5,7 @@ import 'package:flutter/material.dart';
|
||||
import 'package:todo/data/note.dart';
|
||||
import 'package:todo/data/note_repository.dart';
|
||||
import 'package:todo/data/note_template.dart';
|
||||
import 'package:todo/ui/confirm.dart';
|
||||
import 'package:todo/ui/note_detail_screen.dart';
|
||||
|
||||
/// The default status selection: hide completed/dropped work. This is the
|
||||
@ -125,6 +126,11 @@ class _NotesListScreenState extends State<NotesListScreen> {
|
||||
await showModalBottomSheet<void>(
|
||||
context: context,
|
||||
showDragHandle: true,
|
||||
// Scroll-controlled so the sheet can exceed the default 9/16 of the
|
||||
// viewport. On a 768px-tall screen that default is only ~432px, and this
|
||||
// sheet's content grows with the note title and its chip rows -- an
|
||||
// unscrollable sheet would clip the Delete action out of reach.
|
||||
isScrollControlled: true,
|
||||
builder: (_) => _NoteActionsSheet(
|
||||
note: note,
|
||||
onChanged: (updated) async {
|
||||
@ -375,6 +381,12 @@ class _NoteActionsSheetState extends State<_NoteActionsSheet> {
|
||||
foregroundColor: Theme.of(context).colorScheme.error,
|
||||
),
|
||||
onPressed: () async {
|
||||
final confirmed = await confirmDestructive(
|
||||
context,
|
||||
title: 'Delete note?',
|
||||
message: 'This cannot be undone.',
|
||||
);
|
||||
if (!confirmed) return;
|
||||
await widget.onDelete();
|
||||
if (context.mounted) Navigator.of(context).pop();
|
||||
},
|
||||
|
||||
105
test/confirm_test.dart
Normal file
105
test/confirm_test.dart
Normal file
@ -0,0 +1,105 @@
|
||||
import 'package:flutter/material.dart';
|
||||
import 'package:flutter/services.dart';
|
||||
import 'package:flutter_test/flutter_test.dart';
|
||||
import 'package:todo/ui/confirm.dart';
|
||||
|
||||
/// Tests for the destructive-action confirmation.
|
||||
///
|
||||
/// Both delete paths (the actions sheet and the detail screen) used to delete
|
||||
/// immediately with no confirmation and no undo. With a keyboard that is one
|
||||
/// Tab away from a Return the user was about to press anyway, so the defaults
|
||||
/// here — Cancel autofocused, Escape dismisses — are the actual safety
|
||||
/// property, not decoration.
|
||||
void main() {
|
||||
Future<bool?> showFor(WidgetTester tester) async {
|
||||
bool? result;
|
||||
await tester.pumpWidget(
|
||||
MaterialApp(
|
||||
home: Scaffold(
|
||||
body: Builder(
|
||||
builder: (context) => ElevatedButton(
|
||||
onPressed: () async {
|
||||
result = await confirmDestructive(
|
||||
context,
|
||||
title: 'Delete note?',
|
||||
message: 'This cannot be undone.',
|
||||
);
|
||||
},
|
||||
child: const Text('go'),
|
||||
),
|
||||
),
|
||||
),
|
||||
),
|
||||
);
|
||||
await tester.tap(find.text('go'));
|
||||
await tester.pumpAndSettle();
|
||||
return result;
|
||||
}
|
||||
|
||||
testWidgets('shows the title and message', (tester) async {
|
||||
await showFor(tester);
|
||||
expect(find.text('Delete note?'), findsOneWidget);
|
||||
expect(find.text('This cannot be undone.'), findsOneWidget);
|
||||
});
|
||||
|
||||
testWidgets('Cancel is autofocused so a reflexive Return is safe', (
|
||||
tester,
|
||||
) async {
|
||||
await showFor(tester);
|
||||
final cancel = tester.widget<TextButton>(
|
||||
find.ancestor(of: find.text('Cancel'), matching: find.byType(TextButton)),
|
||||
);
|
||||
expect(cancel.autofocus, isTrue);
|
||||
});
|
||||
|
||||
testWidgets('confirming returns true', (tester) async {
|
||||
await showFor(tester);
|
||||
await tester.tap(find.text('Delete'));
|
||||
await tester.pumpAndSettle();
|
||||
expect(find.text('Delete note?'), findsNothing);
|
||||
});
|
||||
|
||||
testWidgets('cancelling returns false', (tester) async {
|
||||
await showFor(tester);
|
||||
await tester.tap(find.text('Cancel'));
|
||||
await tester.pumpAndSettle();
|
||||
expect(find.text('Delete note?'), findsNothing);
|
||||
});
|
||||
|
||||
testWidgets('Escape dismisses without confirming', (tester) async {
|
||||
await showFor(tester);
|
||||
expect(find.text('Delete note?'), findsOneWidget);
|
||||
|
||||
await tester.sendKeyEvent(LogicalKeyboardKey.escape);
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
// A confirmation the keyboard cannot back out of is worse than none.
|
||||
expect(find.text('Delete note?'), findsNothing);
|
||||
});
|
||||
|
||||
testWidgets('custom labels are honoured', (tester) async {
|
||||
await tester.pumpWidget(
|
||||
MaterialApp(
|
||||
home: Scaffold(
|
||||
body: Builder(
|
||||
builder: (context) => ElevatedButton(
|
||||
onPressed: () => confirmDestructive(
|
||||
context,
|
||||
title: 'Discard?',
|
||||
message: 'Unsaved work will be lost.',
|
||||
confirmLabel: 'Discard',
|
||||
cancelLabel: 'Keep editing',
|
||||
),
|
||||
child: const Text('go'),
|
||||
),
|
||||
),
|
||||
),
|
||||
),
|
||||
);
|
||||
await tester.tap(find.text('go'));
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
expect(find.text('Discard'), findsOneWidget);
|
||||
expect(find.text('Keep editing'), findsOneWidget);
|
||||
});
|
||||
}
|
||||
@ -115,7 +115,25 @@ void main() {
|
||||
await tester.tap(find.byTooltip('Delete note'));
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
// Delete is now confirmed. Deleting a note is irreversible and there is no
|
||||
// undo, so a single Return on a focused control must not destroy it.
|
||||
expect(find.text('Delete note?'), findsOneWidget);
|
||||
await tester.tap(find.text('Delete'));
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
expect(await repo.listNotes(), isEmpty);
|
||||
expect(find.byType(NoteDetailScreen), findsNothing);
|
||||
});
|
||||
|
||||
testWidgets('cancelling the delete keeps the note', (tester) async {
|
||||
final repo = await pumpDetail(tester, seedNote('# Keep me'));
|
||||
|
||||
await tester.tap(find.byTooltip('Delete note'));
|
||||
await tester.pumpAndSettle();
|
||||
await tester.tap(find.text('Cancel'));
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
expect(await repo.listNotes(), hasLength(1));
|
||||
expect(find.byType(NoteDetailScreen), findsOneWidget);
|
||||
});
|
||||
}
|
||||
|
||||
@ -109,11 +109,31 @@ void main() {
|
||||
await tester.pump();
|
||||
await tester.pump(const Duration(milliseconds: 300)); // sheet open
|
||||
await tester.tap(find.text('Delete note'));
|
||||
await tester.pump();
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
// Delete is now confirmed on this path too: it is irreversible, has no
|
||||
// undo, and is one Tab from a Return in the sheet's chip rows.
|
||||
expect(find.text('Delete note?'), findsOneWidget);
|
||||
await tester.tap(find.text('Delete'));
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
expect(await repo.listNotes(), isEmpty);
|
||||
});
|
||||
|
||||
testWidgets('cancelling the sheet delete keeps the note', (tester) async {
|
||||
final repo = await pumpList(tester, seed: [note('a', 'Keep me')]);
|
||||
|
||||
await tester.tap(find.byTooltip('Quick actions'));
|
||||
await tester.pump();
|
||||
await tester.pump(const Duration(milliseconds: 300));
|
||||
await tester.tap(find.text('Delete note'));
|
||||
await tester.pumpAndSettle();
|
||||
await tester.tap(find.text('Cancel'));
|
||||
await tester.pumpAndSettle();
|
||||
|
||||
expect(await repo.listNotes(), hasLength(1));
|
||||
});
|
||||
|
||||
testWidgets('per-note sheet changes status via a chip', (tester) async {
|
||||
final repo = await pumpList(tester, seed: [note('a', 'Change me')]);
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user