Add prototype UI for a single-page dialog for adding events/exams. #1286
Conversation
WalkthroughThe recent update introduces a new module for adding events to the timetable, enhancing the app's functionality with conditional navigation. Depending on specific conditions, users can now be directed to either a dialog or a separate page for event addition, streamlining the user experience according to the context. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
feature: timetable / calendar
|
Visit the preview URL for this PR (updated for commit fcc2207): https://sharezone-test--pr1286-new-events-dialog-5zs05fl6.web.app (expires Thu, 08 Feb 2024 13:51:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b |
Jonas-Sander
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (3)
- app/lib/timetable/timetable_add_event/timetable_add_event_dialog.dart (1 hunks)
- app/lib/timetable/timetable_page/timetable_page.dart (2 hunks)
- app/lib/timetable/timetable_page/timetable_page_fab.dart (1 hunks)
Additional comments: 6
app/lib/timetable/timetable_page/timetable_page_fab.dart (1)
- 63-64: Check for the necessity of
await waitingForPopAnimation();when the context is not mounted. This could potentially lead to unwanted behavior if the context becomes unmounted before the animation completes.app/lib/timetable/timetable_page/timetable_page.dart (1)
- 35-35: The import of
timetable_add_event/timetable_add_event_dialog.dartis added. Ensure that this module is utilized within the file or consider removing the import if it's not needed.app/lib/timetable/timetable_add_event/timetable_add_event_dialog.dart (4)
- 215-227: Placeholder text and commented-out error handling in
_TitleFieldBase. Ensure that proper error handling and user feedback mechanisms are implemented.- 305-309: The
CourseTileBasewidget is used with null parameters forcourseNameanderrorText. Ensure these values are intentionally left null or provide meaningful defaults or dynamic values.- 408-408: The use of
date?.parser.toYMMMEdmight throw an error ifdateis null. Ensure thatdateis always non-null when accessed or handle the potential null case explicitly.- 527-527: The
onChangedcallback in_Locationis a no-op ((_) {}). If this is intentional for a prototype, consider adding a comment explaining future plans. Otherwise, implement the callback or remove it if unnecessary.
| // ignore: dead_code | ||
| if (false) { | ||
| await Navigator.push( | ||
| context, | ||
| IgnoreWillPopScopeWhenIosSwipeBackRoute( | ||
| builder: (context) => TimetableAddEventDialog( | ||
| isExam: isExam, | ||
| ), | ||
| settings: const RouteSettings(name: TimetableAddEventDialog.tag))); | ||
| return null; | ||
| } else { |
There was a problem hiding this comment.
The use of // ignore: dead_code and a hardcoded false condition in the if statement is not ideal for maintainability or readability. Consider replacing this with a feature flag or configuration parameter that can be easily toggled.
| // ignore: dead_code | ||
| if (false) { | ||
| await Navigator.push( | ||
| context, | ||
| IgnoreWillPopScopeWhenIosSwipeBackRoute( | ||
| builder: (context) => TimetableAddEventDialog( | ||
| isExam: isExam, | ||
| ), | ||
| settings: const RouteSettings(name: TimetableAddEventDialog.tag))); | ||
| return null; | ||
| } else { | ||
| final result = await Navigator.push<TimetableResult>( | ||
| context, | ||
| IgnoreWillPopScopeWhenIosSwipeBackRoute( | ||
| builder: (context) => TimetableAddEventPage(isExam: isExam), | ||
| settings: const RouteSettings(name: TimetableAddEventPage.tag))); | ||
| if (result != null) { | ||
| await waitingForPopAnimation(); | ||
| if (!context.mounted) return null; | ||
|
|
||
| showDataArrivalConfirmedSnackbar(context: context); | ||
| showDataArrivalConfirmedSnackbar(context: context); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
The await Navigator.push inside the if block is never reached due to the hardcoded false condition. This dead code should either be removed or updated to reflect the intended functionality.
|
|
||
| showDataArrivalConfirmedSnackbar(context: context); | ||
| showDataArrivalConfirmedSnackbar(context: context); |
There was a problem hiding this comment.
The function showDataArrivalConfirmedSnackbar is called without checking the context.mounted status afterward, which could lead to attempting to show a snackbar in an unmounted context.
+ if (context.mounted) {
showDataArrivalConfirmedSnackbar(context: context);
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| showDataArrivalConfirmedSnackbar(context: context); | |
| if (context.mounted) { | |
| showDataArrivalConfirmedSnackbar(context: context); | |
| } |
| // final hasInputChanged = hasModifiedData(); | ||
| const hasInputChanged = false; | ||
| final navigator = Navigator.of(context); | ||
| if (!hasInputChanged) { | ||
| navigator.pop(); | ||
| return; | ||
| } | ||
|
|
||
| // final shouldPop = await warnUserAboutLeavingForm(context); | ||
| // if (shouldPop && context.mounted) { | ||
| // navigator.pop(); | ||
| // } |
There was a problem hiding this comment.
Commented-out code related to checking if input has changed and warning the user about leaving the form. Consider removing or implementing this functionality if needed.
| import 'package:sharezone_widgets/sharezone_widgets.dart'; | ||
| import 'package:time/time.dart'; | ||
|
|
||
| final _titleNode = FocusNode(); |
There was a problem hiding this comment.
The global variable _titleNode is defined outside of any class. Consider moving it inside the relevant widget to encapsulate its scope and prevent potential conflicts or misuse.
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| if (PlatformCheck.isDesktopOrWeb) return const SizedBox(height: 4); |
There was a problem hiding this comment.
The use of PlatformCheck.isDesktopOrWeb to conditionally render a widget could be refined by abstracting platform checks into a separate utility class or function for better maintainability and testing.
| Future<void> onPressed(BuildContext context) async { | ||
| Navigator.pop(context); | ||
| // final bloc = bloc_lib.BlocProvider.of<HomeworkDialogBloc>(context); | ||
| // try { | ||
| // bloc.add(const Save()); | ||
| // } on Exception catch (e) { | ||
| // log("Exception when submitting: $e", error: e); | ||
| // showSnackSec( | ||
| // text: | ||
| // "Es gab einen unbekannten Fehler (${e.toString()}) 😖 Bitte kontaktiere den Support!", | ||
| // context: context, | ||
| // seconds: 5, | ||
| // ); | ||
| // } |
There was a problem hiding this comment.
Commented-out code within the onPressed method of _SaveButton. If this logic is planned for future use, consider adding a TODO comment; otherwise, remove the commented-out code to clean up the file.
| if (false) ...[ | ||
| Row( | ||
| children: [ | ||
| const SizedBox(width: 34), | ||
| OutlinedButton( | ||
| onPressed: () { | ||
| Navigator.push( | ||
| context, | ||
| MaterialPageRoute( | ||
| builder: (context) => const _LessonPickerPage(), | ||
| ), | ||
| ); | ||
| }, | ||
| style: OutlinedButton.styleFrom( | ||
| shape: RoundedRectangleBorder( | ||
| borderRadius: BorderRadius.circular(14))), | ||
| child: const Text('Schulstunde auswählen'), | ||
| ), | ||
| ], | ||
| ), | ||
| const SizedBox(height: 10), | ||
| ], |
There was a problem hiding this comment.
Dead code within if (false) block in _DateAndTimePicker. Consider removing or implementing the intended functionality.
This PR adds a a prototype UI for adding events/exams. Pressing buttons etc doesn't do anything yet.
I copied most of the code it from
homework_dialog.dart. In the next PR I want to move shared widgets out tosharezone_widgets. I commented some stuff out so I don't forget some functionality in the future.Since the new dialog doesn't work yet and we can't see in the code if we're in a preview, I disabled showing this dialog for now.
Summary by CodeRabbit