From 3238fc2c4ac32107fe35abec49e66102f15369cc Mon Sep 17 00:00:00 2001 From: Joey Boerwinkel Date: Mon, 15 Jul 2024 11:30:16 +0200 Subject: [PATCH] chore: apply code review comments --- .../lib/src/config/availability_options.dart | 1 + .../lib/src/service/initial_data.dart | 2 ++ .../lib/src/service/local_data_interface.dart | 3 +++ .../ui/screens/availability_modification.dart | 17 +++++++++++++++++ .../src/ui/screens/availability_overview.dart | 4 ++++ .../ui/screens/template_day_modification.dart | 6 ++++++ .../lib/src/ui/screens/template_overview.dart | 8 ++++++++ .../availability_template_selection.dart | 6 ++++++ .../lib/src/ui/widgets/calendar_grid.dart | 4 ++++ .../lib/src/ui/widgets/pause_selection.dart | 2 ++ .../lib/src/ui/widgets/template_legend.dart | 12 ++++++++++++ .../lib/src/util/availability_deviation.dart | 2 ++ 12 files changed, 67 insertions(+) diff --git a/packages/flutter_availability/lib/src/config/availability_options.dart b/packages/flutter_availability/lib/src/config/availability_options.dart index 1ce2482..c93decb 100644 --- a/packages/flutter_availability/lib/src/config/availability_options.dart +++ b/packages/flutter_availability/lib/src/config/availability_options.dart @@ -24,6 +24,7 @@ class AvailabilityOptions { this.colors = const AvailabilityColors(), this.confirmationDialogBuilder = DefaultConfirmationDialog.builder, this.timePickerBuilder, + // TODO(Joey): Also have a DefaultLoader.builder this.loadingIndicatorBuilder = defaultLoader, AvailabilityDataInterface? dataInterface, }) : dataInterface = dataInterface ?? LocalAvailabilityDataInterface(); diff --git a/packages/flutter_availability/lib/src/service/initial_data.dart b/packages/flutter_availability/lib/src/service/initial_data.dart index 0e166b2..4457510 100644 --- a/packages/flutter_availability/lib/src/service/initial_data.dart +++ b/packages/flutter_availability/lib/src/service/initial_data.dart @@ -15,6 +15,8 @@ List getDefaultLocalAvailabilitiesForUser() { AvailabilityModel( id: availability.$1, userId: "", + // TODO(Joey): Even though this is testdata, Never use .add(duration) + // to move to the next day. startDate: currentMonth.add(Duration(days: availability.$2)), endDate: currentMonth.add(Duration(days: availability.$2)), breaks: [], diff --git a/packages/flutter_availability/lib/src/service/local_data_interface.dart b/packages/flutter_availability/lib/src/service/local_data_interface.dart index 6a53666..b389adb 100644 --- a/packages/flutter_availability/lib/src/service/local_data_interface.dart +++ b/packages/flutter_availability/lib/src/service/local_data_interface.dart @@ -207,6 +207,7 @@ class LocalAvailabilityDataInterface implements AvailabilityDataInterface { return availabilities[index]; } } + // TODO(Joey): Throw proper exceptions here throw Exception("Availability not found"); } @@ -225,6 +226,8 @@ class LocalAvailabilityDataInterface implements AvailabilityDataInterface { return templates[index]; } } + // TODO(Joey): Add proper exceptions in the data interface and throw them + // here throw Exception("Template not found"); } diff --git a/packages/flutter_availability/lib/src/ui/screens/availability_modification.dart b/packages/flutter_availability/lib/src/ui/screens/availability_modification.dart index e028fed..761d454 100644 --- a/packages/flutter_availability/lib/src/ui/screens/availability_modification.dart +++ b/packages/flutter_availability/lib/src/ui/screens/availability_modification.dart @@ -54,6 +54,8 @@ class _AvailabilitiesModificationScreenState @override void initState() { super.initState(); + // TODO(Joey): These can be immediately assigned to the properties + // This removes the need for an initState _availability = widget.initialAvailabilities.getAvailabilities().firstOrNull ?? AvailabilityModel( @@ -76,6 +78,8 @@ class _AvailabilitiesModificationScreenState // TODO(freek): the selected period might be longer than 1 month //so we need to get all the availabilites through a stream + // TODO(Joey): separate logic from layout to adhere to the single + // responsibility principle Future onSave() async { if (_clearAvailability) { await service.clearAvailabilities( @@ -100,11 +104,15 @@ class _AvailabilitiesModificationScreenState } Future onClickSave() async { + // TODO(Joey): The name confirmationDialogBuilder does not represent the + // expected implementation. var confirmed = await options.confirmationDialogBuilder( context, title: translations.availabilityDialogConfirmTitle, description: translations.availabilityDialogConfirmDescription, ); + // TODO(Joey): We should make the interface of the dialog function return + // a non nullable bool. Now we are implicitly setting default behaviour if (confirmed ?? false) { await onSave(); } @@ -121,6 +129,7 @@ class _AvailabilitiesModificationScreenState var clearSection = AvailabilityClearSection( range: widget.dateRange, clearAvailable: _clearAvailability, + // TODO(Joey): Extract this function onChanged: (isChecked) { setState(() { _clearAvailability = isChecked; @@ -130,6 +139,7 @@ class _AvailabilitiesModificationScreenState var templateSelection = AvailabilityTemplateSelection( selectedTemplates: _selectedTemplates, + // TODO(Joey): Extract this function onTemplateAdd: () async { var template = await widget.onTemplateSelection(); if (template != null) { @@ -138,6 +148,7 @@ class _AvailabilitiesModificationScreenState }); } }, + // TODO(Joey): Extract these functions onTemplatesRemoved: () { setState(() { _selectedTemplates = []; @@ -150,9 +161,11 @@ class _AvailabilitiesModificationScreenState startTime: _startTime, endTime: _endTime, key: ValueKey([_startTime, _endTime]), + // TODO(Joey): Extract these onStartChanged: (start) => setState(() { _startTime = start; }), + // TODO(Joey): Extract these onEndChanged: (end) => setState(() { _endTime = end; }), @@ -160,6 +173,7 @@ class _AvailabilitiesModificationScreenState var pauseSelection = PauseSelection( breaks: _availability.breaks, + // TODO(Joey): Extract these onBreaksChanged: (breaks) { setState(() { _availability = _availability.copyWith(breaks: breaks); @@ -167,6 +181,8 @@ class _AvailabilitiesModificationScreenState }, ); + // TODO(Joey): this structure is defined multiple times, we should create + // a widget to handle this consistently var body = CustomScrollView( slivers: [ SliverPadding( @@ -181,6 +197,7 @@ class _AvailabilitiesModificationScreenState templateSelection, const SizedBox(height: 24), timeSelection, + // TODO(Joey): Not divisible by 4 const SizedBox(height: 26), pauseSelection, ], diff --git a/packages/flutter_availability/lib/src/ui/screens/availability_overview.dart b/packages/flutter_availability/lib/src/ui/screens/availability_overview.dart index 4d3c273..7a94991 100644 --- a/packages/flutter_availability/lib/src/ui/screens/availability_overview.dart +++ b/packages/flutter_availability/lib/src/ui/screens/availability_overview.dart @@ -51,6 +51,7 @@ class _AvailabilityOverviewState extends State { var availabilitySnapshot = useStream(availabilityStream); + // TODO(Joey): Way too complex of a function var selectedAvailabilities = _selectedRange != null ? availabilitySnapshot.data ?.where( @@ -93,6 +94,7 @@ class _AvailabilityOverviewState extends State { availabilities: availabilitySnapshot, ); + // TODO(Joey): too complex of a definition for the function var onButtonPress = _selectedRange == null ? null : () { @@ -111,6 +113,7 @@ class _AvailabilityOverviewState extends State { title: translations.clearAvailabilityConfirmTitle, description: translations.clearAvailabilityConfirmDescription, ); + // TODO(Joey): Expect a non nullable if (confirmed ?? false) { await service .clearAvailabilities(selectedAvailabilities.getAvailabilities()); @@ -132,6 +135,7 @@ class _AvailabilityOverviewState extends State { Text(translations.editAvailabilityButton), ); + // TODO(Joey): This structure is defined multiple times var body = CustomScrollView( slivers: [ SliverPadding( diff --git a/packages/flutter_availability/lib/src/ui/screens/template_day_modification.dart b/packages/flutter_availability/lib/src/ui/screens/template_day_modification.dart index 97299c7..a280838 100644 --- a/packages/flutter_availability/lib/src/ui/screens/template_day_modification.dart +++ b/packages/flutter_availability/lib/src/ui/screens/template_day_modification.dart @@ -104,12 +104,15 @@ class _DayTemplateModificationScreenState var timeSection = TemplateTimeSelection( key: ValueKey(_template.templateData), + // TODO(Joey): Extract this startTime: TimeOfDay.fromDateTime( (_template.templateData as DayTemplateData).startTime, ), + // TODO(Joey): Extract this endTime: TimeOfDay.fromDateTime( (_template.templateData as DayTemplateData).endTime, ), + // TODO(Joey): Extract this onStartChanged: (start) { var startTime = (_template.templateData as DayTemplateData).startTime; var updatedStartTime = DateTime( @@ -127,6 +130,7 @@ class _DayTemplateModificationScreenState ); }); }, + // TODO(Joey): Extract this onEndChanged: (end) { var endTime = (_template.templateData as DayTemplateData).endTime; var updatedEndTime = DateTime( @@ -148,6 +152,7 @@ class _DayTemplateModificationScreenState var colorSection = TemplateColorSelection( selectedColor: _selectedColor, + // TODO(Joey): Extract this onColorSelected: (color) { setState(() { _selectedColor = color; @@ -158,6 +163,7 @@ class _DayTemplateModificationScreenState var pauseSection = PauseSelection( breaks: (_template.templateData as DayTemplateData).breaks, + // TODO(Joey): Extrac this onBreaksChanged: (breaks) { setState(() { _template = _template.copyWith( diff --git a/packages/flutter_availability/lib/src/ui/screens/template_overview.dart b/packages/flutter_availability/lib/src/ui/screens/template_overview.dart index c477208..3179ce2 100644 --- a/packages/flutter_availability/lib/src/ui/screens/template_overview.dart +++ b/packages/flutter_availability/lib/src/ui/screens/template_overview.dart @@ -100,6 +100,7 @@ class _TemplateListSection extends StatelessWidget { final String sectionTitle; final String createButtonText; + // transform the stream to a snapshot as low as possible to reduce rebuilds final AsyncSnapshot> templatesSnapshot; final void Function(AvailabilityTemplateModel template) onEditTemplate; final VoidCallback onAddTemplate; @@ -147,8 +148,13 @@ class _TemplateListSection extends StatelessWidget { Text(sectionTitle, style: textTheme.titleMedium), const SizedBox(height: 8), const Divider(height: 1), + // TODO(Joey): Do not make this nullable, in the build make sure to + // have the expected value ready. for (var template in templatesSnapshot.data ?? []) ...[ + // TODO(Joey): Extract this as a widget + // TODO(Joey): Do not simply use gesture detectors, always think of + // semantics, interaction and other UX GestureDetector( onTap: () => onClickTemplate(template), child: Container( @@ -171,6 +177,8 @@ class _TemplateListSection extends StatelessWidget { const SizedBox(width: 8), Text(template.name, style: textTheme.bodyLarge), const Spacer(), + // TODO(Joey): Do not simply use gesture detectors, always + // think of semantics, interaction and other UX GestureDetector( onTap: () => onEditTemplate(template), child: const Icon(Icons.edit), diff --git a/packages/flutter_availability/lib/src/ui/widgets/availability_template_selection.dart b/packages/flutter_availability/lib/src/ui/widgets/availability_template_selection.dart index cd3b5b7..7cc294d 100644 --- a/packages/flutter_availability/lib/src/ui/widgets/availability_template_selection.dart +++ b/packages/flutter_availability/lib/src/ui/widgets/availability_template_selection.dart @@ -35,6 +35,7 @@ class AvailabilityTemplateSelection extends StatelessWidget { var options = availabilityScope.options; var translations = options.translations; + // TODO(Joey): Do not nest ternairy operators var titleText = selectedTemplates.isEmpty ? translations.availabilityAddTemplateTitle : selectedTemplates.length > 1 @@ -62,6 +63,7 @@ class AvailabilityTemplateSelection extends StatelessWidget { if (selectedTemplates.isEmpty) ...[ addButton, ] else ...[ + // TODO(Joey): Extract this as a widget Container( padding: const EdgeInsets.all(12), decoration: BoxDecoration( @@ -69,6 +71,9 @@ class AvailabilityTemplateSelection extends StatelessWidget { color: theme.colorScheme.primary, width: 1, ), + // TODO(Joey): This seems like a repeating borderRadius. I can + // understand if these are not configurable, but I do think that + // they should be defined only once. borderRadius: BorderRadius.circular(5), ), child: Row( @@ -79,6 +84,7 @@ class AvailabilityTemplateSelection extends StatelessWidget { crossAxisAlignment: CrossAxisAlignment.start, children: [ for (var template in selectedTemplates) ...[ + // TODO(Joey): Extract this as a widget Row( children: [ Container( diff --git a/packages/flutter_availability/lib/src/ui/widgets/calendar_grid.dart b/packages/flutter_availability/lib/src/ui/widgets/calendar_grid.dart index 23f7e4c..c55bec3 100644 --- a/packages/flutter_availability/lib/src/ui/widgets/calendar_grid.dart +++ b/packages/flutter_availability/lib/src/ui/widgets/calendar_grid.dart @@ -73,6 +73,7 @@ class CalendarGrid extends StatelessWidget { mainAxisSpacing: 12, ), itemBuilder: (context, index) { + // TODO(Joey): Extract all this as a widget var day = calendarDays[index]; var dayColor = day.color ?? colors.customAvailabilityColor ?? @@ -86,6 +87,7 @@ class CalendarGrid extends StatelessWidget { ); var textStyle = textTheme.bodyLarge?.copyWith(color: textColor); + // TODO(Joey): Watch out for using gesture detectors return GestureDetector( onTap: () => onDayTap(day.date), child: DecoratedBox( @@ -186,6 +188,8 @@ List _generateCalendarDays( required bool isNextMonth, }) { for (var i = 0; i < count; i++) { + // TODO(Joey): Never increase days with duration, always use internal + // datetime math. var day = isNextMonth ? startDay.add(Duration(days: i + 1)) : startDay.subtract(Duration(days: count - i)); diff --git a/packages/flutter_availability/lib/src/ui/widgets/pause_selection.dart b/packages/flutter_availability/lib/src/ui/widgets/pause_selection.dart index 3098b16..435c844 100644 --- a/packages/flutter_availability/lib/src/ui/widgets/pause_selection.dart +++ b/packages/flutter_availability/lib/src/ui/widgets/pause_selection.dart @@ -140,6 +140,7 @@ class BreakDisplay extends StatelessWidget { TimeOfDay.fromDateTime(breakModel.endTime), ); + // TODO(Joey): Watch out with gesture detectors return GestureDetector( onTap: onClick, child: Container( @@ -158,6 +159,7 @@ class BreakDisplay extends StatelessWidget { "$endTime", ), const Spacer(), + // TODO(Joey): Watch out with gesturedetectors GestureDetector(onTap: onRemove, child: const Icon(Icons.remove)), ], ), diff --git a/packages/flutter_availability/lib/src/ui/widgets/template_legend.dart b/packages/flutter_availability/lib/src/ui/widgets/template_legend.dart index 27b9ba3..3e98379 100644 --- a/packages/flutter_availability/lib/src/ui/widgets/template_legend.dart +++ b/packages/flutter_availability/lib/src/ui/widgets/template_legend.dart @@ -110,11 +110,15 @@ class _TemplateLegendState extends State { ), padding: const EdgeInsets.only(right: 2), child: _templateDrawerOpen && !templatesLoading + // TODO(Joey): A listview inside a scrollview inside the + // scrollable that each page has seems like really strange UX + // TODO(Joey): No ternary operators in the layout ? SingleChildScrollView( child: Column( children: [ Container( constraints: const BoxConstraints( + // TODO(Joey): Not divisible by 4 maxHeight: 150, ), child: Scrollbar( @@ -125,9 +129,12 @@ class _TemplateLegendState extends State { child: ListView.builder( controller: _scrollController, shrinkWrap: true, + // TODO(Joey): This seems like an odd way to + // implement appending items itemCount: templates.length + 2, itemBuilder: (context, index) { if (index == 0) { + // TODO(Joey): Extract this as a widget return Column( children: [ _TemplateLegendItem( @@ -175,6 +182,9 @@ class _TemplateLegendState extends State { thickness: 1, ), ), + // TODO(Joey): This is too complex of a check to read the layout + // There are 8 different combinations parameters with 2 different + // outcomes if (!templatesAvailable && (!_templateDrawerOpen || templatesLoading)) ...[ const SizedBox(height: 12), @@ -213,6 +223,7 @@ class _TemplateLegendItem extends StatelessWidget { Container( decoration: BoxDecoration( color: backgroundColor, + // TODO(Joey): Use a global borderRadius borderRadius: BorderRadius.circular(5), border: Border.all( color: borderColor ?? Colors.transparent, @@ -221,6 +232,7 @@ class _TemplateLegendItem extends StatelessWidget { width: 20, height: 20, ), + // TODO(Joey): Not divisible by 4 const SizedBox(width: 6), Text(name, style: Theme.of(context).textTheme.bodyLarge), ], diff --git a/packages/flutter_availability/lib/src/util/availability_deviation.dart b/packages/flutter_availability/lib/src/util/availability_deviation.dart index ffd88ed..68af7d5 100644 --- a/packages/flutter_availability/lib/src/util/availability_deviation.dart +++ b/packages/flutter_availability/lib/src/util/availability_deviation.dart @@ -11,6 +11,8 @@ bool isTemplateDeviated( DateTime? templateStartDate; DateTime? templateEndDate; + // TODO(Joey): Add a method to a templateModel: getEndTimeForDayOfWeek() + // as well as for start time. Allow polymorphism to resolve this if statement if (template.templateType == AvailabilityTemplateType.week) { templateStartDate = (template.templateData as WeekTemplateData) .data[WeekDay.values[dayOfWeek - 1]]