Restructure itinerary and add the concept of "choosing" specific details #274
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the travel app's UI components and AI interaction logic. The replacement of ItineraryItem with the more structured ItineraryDay and ItineraryEntry is a great improvement for building detailed itineraries. The addition of CheckboxFilterChipsInput and state management for input fields also enhances the user experience. My review focuses on a few areas for improvement: correcting a recurring typo, fixing a minor grammatical error in the AI prompt, improving state encapsulation in a new widget, and a suggestion to implement logging where it's already been noted as a good idea.
|
|
||
| import '../widgets/dismiss_notification.dart'; | ||
|
|
||
| enum ItineraryEntryType { accomodation, transport, activity } |
There was a problem hiding this comment.
There's a typo in accomodation. It should be accommodation. Since this is part of an enum that defines an API, it's important to correct it. Remember to update all usages, including in the _getIconForType method and in the AI prompt in main.dart.
| enum ItineraryEntryType { accomodation, transport, activity } | |
| enum ItineraryEntryType { accommodation, transport, activity } |
| } catch (e) { | ||
| // Invalid icon name, default to no icon. | ||
| // Consider logging this error. | ||
| icon = null; | ||
| } |
There was a problem hiding this comment.
The comment on line 99 suggests logging this error, which is a great idea for debugging. It would be beneficial to actually implement this. You could use a logging package like package:logging to record the exception when an invalid icon name is provided.
There was a problem hiding this comment.
Don't worry about this right now - I am adding a logging library in a separate PR
|
/gemini summarize |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the itinerary structure, replacing ItineraryItem with a more organized ItineraryDay and ItineraryEntry system. The changes are comprehensive, including updates to the AI prompt, widget catalog, and the addition of new components like CheckboxFilterChipsInput along with corresponding tests. My feedback focuses on a couple of typos that could impact the AI's performance and a suggestion to improve the usability and efficiency of the new checkbox input component.
| return Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: widget.options.map((option) { | ||
| return CheckboxListTile( | ||
| title: Text(option), | ||
| value: tempSelectedOptions.contains(option), | ||
| onChanged: (bool? newValue) { | ||
| setModalState(() { | ||
| if (newValue == true) { | ||
| tempSelectedOptions.add(option); | ||
| } else { | ||
| tempSelectedOptions.remove(option); | ||
| } | ||
| }); | ||
| setState(() { | ||
| _selectedOptions = tempSelectedOptions; | ||
| }); | ||
| widget.dispatchEvent( | ||
| UiChangeEvent( | ||
| widgetId: widget.widgetId, | ||
| eventType: 'filterOptionsSelected', | ||
| value: _selectedOptions, | ||
| ), | ||
| ); | ||
| }, | ||
| ); | ||
| }).toList(), | ||
| ); |
There was a problem hiding this comment.
The current implementation dispatches an event on every checkbox change, which can be inefficient and lead to a high volume of events. It also complicates state management by using both setState and setModalState in the onChanged callback.
A better approach is to add a confirmation button to the bottom sheet. This allows the user to make all their selections first and then confirm their choices with a single action. This simplifies state management and ensures only a single, consolidated event is dispatched.
Here's how you could refactor it:
- In
onChanged, only callsetModalStateto update the temporary selection. - Add a button (e.g., 'Done') to the bottom sheet.
- In the button's
onPressedhandler, callsetStateto update the widget's state, dispatch the event, and then close the modal.
return Column(
mainAxisSize: MainAxisSize.min,
children: [
...widget.options.map((option) {
return CheckboxListTile(
title: Text(option),
value: tempSelectedOptions.contains(option),
onChanged: (bool? newValue) {
setModalState(() {
if (newValue == true) {
tempSelectedOptions.add(option);
} else {
tempSelectedOptions.remove(option);
}
});
},
);
}),
Padding(
padding: const EdgeInsets.all(16.0),
child: ElevatedButton(
onPressed: () {
setState(() {
_selectedOptions = tempSelectedOptions;
});
widget.dispatchEvent(
UiChangeEvent(
widgetId: widget.widgetId,
eventType: 'filterOptionsSelected',
value: _selectedOptions,
),
);
Navigator.pop(context);
},
child: const Text('Done'),
),
),
],
);There was a problem hiding this comment.
Don't fix this. We will update to the state mechanism later.
Summary of ChangesThis pull request primarily focuses on enhancing the travel application's UI component catalog and the AI's ability to construct dynamic user interfaces. The changes introduce more structured itinerary components and a new multi-selection input widget, providing greater flexibility and control for generating and managing travel plans. Highlights
Changelog
Activity
|
jacobsimionato
changed the title
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly refactors the itinerary structure by replacing the monolithic ItineraryItem with a more granular and expressive combination of ItineraryDay and ItineraryEntry. This change improves the data model and allows for more detailed travel plans. The introduction of CheckboxFilterChipsInput for multi-select options and the centralization of icons in common.dart are also welcome improvements. The AI prompts have been updated accordingly to leverage these new components. My review identifies a state management bug in the new checkbox widget, suggests an improvement for code consistency, and points out a minor naming issue in the new tests. Overall, this is a well-executed enhancement to the application.
examples/travel_app/lib/src/catalog/checkbox_filter_chips_input.dart
Outdated
Show resolved
Hide resolved
| Row( | ||
| children: [ | ||
| const Icon(Icons.access_time, size: 16.0), | ||
| const SizedBox(width: 4.0), | ||
| Text(time, style: theme.textTheme.bodyMedium), | ||
| ], | ||
| ), | ||
| if (address != null) ...[ | ||
| const SizedBox(height: 4.0), | ||
| Row( | ||
| children: [ | ||
| const Icon(Icons.location_on, size: 16.0), | ||
| const SizedBox(width: 4.0), | ||
| Expanded( | ||
| child: Text( | ||
| address!, | ||
| style: theme.textTheme.bodyMedium, | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ], |
There was a problem hiding this comment.
This widget uses hardcoded Icon widgets for time and address. The newly introduced common.dart file provides a centralized iconFor utility function and a TravelIcon enum that includes icons for time and location.
To improve consistency and maintainability, consider using the shared iconFor function from common.dart for these icons. You will need to add an import for common.dart.
There was a problem hiding this comment.
Let's not worry about this right now.
|
I have addressed the feedback. Please take another look. |
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of restructuring the itinerary components into a more granular and maintainable ItineraryDay and ItineraryEntry structure. The introduction of the CheckboxFilterChipsInput for multi-selection and the common.dart file for shared utilities are also excellent additions that improve the app's functionality and code quality.
My review includes a few suggestions:
- A high-severity recommendation to improve the user experience of the new
CheckboxFilterChipsInputby avoiding excessive event dispatches. - A couple of medium-severity comments to fix typos in the AI prompt.
- A medium-severity suggestion to improve code consistency by using the new shared icon utility in the
ItineraryEntrywidget.
Overall, these are solid changes. Addressing these points will further enhance the quality of the code.
| return Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: widget.options.map((option) { | ||
| return CheckboxListTile( | ||
| title: Text(option), | ||
| value: tempSelectedOptions.contains(option), | ||
| onChanged: (bool? newValue) { | ||
| setModalState(() { | ||
| if (newValue == true) { | ||
| tempSelectedOptions.add(option); | ||
| } else { | ||
| tempSelectedOptions.remove(option); | ||
| } | ||
| }); | ||
| setState(() { | ||
| _selectedOptions = List.from(tempSelectedOptions); | ||
| }); | ||
| widget.dispatchEvent( | ||
| UiChangeEvent( | ||
| widgetId: widget.widgetId, | ||
| eventType: 'filterOptionsSelected', | ||
| value: tempSelectedOptions, | ||
| ), | ||
| ); | ||
| }, | ||
| ); | ||
| }).toList(), | ||
| ); |
There was a problem hiding this comment.
Dispatching a UiChangeEvent on every checkbox tap can trigger an excessive number of AI inferences, leading to performance issues and a potentially confusing user experience as the UI might try to update rapidly. A better approach is to collect all the user's selections and send them in a single event.
I suggest adding a confirmation button (e.g., 'Apply') to the bottom sheet. The checkboxes should only update the local state of the modal. When the user presses the 'Apply' button, you can then update the chip's state and dispatch a single UiChangeEvent with all the selected options.
return Column(
mainAxisSize: MainAxisSize.min,
children: [
Flexible(
child: ListView(
shrinkWrap: true,
children: widget.options.map((option) {
return CheckboxListTile(
title: Text(option),
value: tempSelectedOptions.contains(option),
onChanged: (bool? newValue) {
setModalState(() {
if (newValue == true) {
tempSelectedOptions.add(option);
} else {
tempSelectedOptions.remove(option);
}
});
},
);
}).toList(),
),
),
Padding(
padding: const EdgeInsets.all(16.0),
child: ElevatedButton(
style: ElevatedButton.styleFrom(
minimumSize: const Size(double.infinity, 48),
),
onPressed: () {
setState(() {
_selectedOptions = List.from(tempSelectedOptions);
});
widget.dispatchEvent(
UiChangeEvent(
widgetId: widget.widgetId,
eventType: 'filterOptionsSelected',
value: tempSelectedOptions,
),
);
Navigator.pop(context);
},
child: const Text('Apply'),
),
),
],
);| IconData _getIconForType(ItineraryEntryType type) { | ||
| switch (type) { | ||
| case ItineraryEntryType.accommodation: | ||
| return Icons.hotel; | ||
| case ItineraryEntryType.transport: | ||
| return Icons.train; | ||
| case ItineraryEntryType.activity: | ||
| return Icons.local_activity; | ||
| } | ||
| } |
There was a problem hiding this comment.
This local _getIconForType function is inconsistent with the new approach of centralizing icons in common.dart using the TravelIcon enum and iconFor function. To improve maintainability, this logic should be integrated with the common utility.
I recommend the following:
- Add
activityto theTravelIconenum incommon.dart. - Map it to
Icons.local_activityin theiconForfunction. - Here, import
common.dartand replace this switch statement with calls toiconFor, after mappingItineraryEntryTypeto the correspondingTravelIcon.
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the itinerary feature, introducing a more granular structure with ItineraryDay and ItineraryEntry, and adding a new CheckboxFilterChipsInput for multi-selection. The changes improve the application's structure and capabilities. My review focuses on further refining the new components, with suggestions to improve error handling, user experience, event dispatching efficiency, and code consistency. Overall, these are great improvements.
| } catch (e) { | ||
| // Invalid icon name, default to no icon. | ||
| // Consider logging this error. | ||
| icon = null; | ||
| } |
There was a problem hiding this comment.
The catch block currently silences the error when an invalid icon name is provided from the AI. As the code comment suggests, it's better to log this error. Silently failing can make debugging difficult if icons don't appear as expected, especially since the iconName is generated by the AI and could be unpredictable.
} catch (e, s) {
// Invalid icon name, default to no icon.
// Consider logging this error.
debugPrint('Invalid icon name: ${checkboxFilterChipsData.iconName}\n$e\n$s');
icon = null;
}| return FilterChip( | ||
| avatar: widget.icon != null ? Icon(widget.icon) : null, | ||
| label: Text(_chipLabel), | ||
| selected: false, |
There was a problem hiding this comment.
The FilterChip's selected property is hardcoded to false. This means the chip never visually indicates that options have been chosen. To provide better visual feedback to the user, the selected state should reflect whether any options are selected.
selected: _selectedOptions.isNotEmpty,| onChanged: (bool? newValue) { | ||
| setModalState(() { | ||
| if (newValue == true) { | ||
| tempSelectedOptions.add(option); | ||
| } else { | ||
| tempSelectedOptions.remove(option); | ||
| } | ||
| }); | ||
| setState(() { | ||
| _selectedOptions = List.from(tempSelectedOptions); | ||
| }); | ||
| widget.dispatchEvent( | ||
| UiChangeEvent( | ||
| widgetId: widget.widgetId, | ||
| eventType: 'filterOptionsSelected', | ||
| value: tempSelectedOptions, | ||
| ), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
The onChanged callback dispatches a UiChangeEvent every time a user toggles a checkbox. This can lead to a high volume of events sent to the AI, which might be inefficient and costly, especially if a user is quickly selecting multiple items.
Consider batching these changes. A common pattern is to add a "Done" or "Apply" button to the bottom sheet. The event would then be dispatched once with the final selection when the user is finished. This would make the interaction more efficient and reduce the load on the AI.
| IconData _getIconForType(ItineraryEntryType type) { | ||
| switch (type) { | ||
| case ItineraryEntryType.accommodation: | ||
| return Icons.hotel; | ||
| case ItineraryEntryType.transport: | ||
| return Icons.train; | ||
| case ItineraryEntryType.activity: | ||
| return Icons.local_activity; | ||
| } | ||
| } |
There was a problem hiding this comment.
This widget defines its own _getIconForType method, which duplicates logic that is being centralized in common.dart. To improve maintainability and code consistency, this method should be refactored to use the new iconFor utility. This would likely involve adding an activity icon to the TravelIcon enum in common.dart to make the icon management fully centralized.
2 participants
Highlights
ItineraryItemwidget has been replaced by a more granular structure usingItineraryDayandItineraryEntryto better organize daily activities and bookings within a travel itinerary.CheckboxFilterChipsInputwidget has been introduced, allowing users to select multiple options from a predefined list, enhancing user interaction for preferences like amenities or dietary restrictions.main.darthave been updated to reflect the new itinerary structure, guide the AI in generating more detailed itineraries with booking statuses, and incorporate new state management guidelines.common.dartfile has been added to centralize shared enums likeTravelIconand utility functions, promoting code reusability.Changelog
inputGroup,itineraryWithDetails,itineraryEntry, andtravelCarouselwidgets, and added new guidelines for itinerary structure and state management.travelAppCatalogdefinition to includecheckboxFilterChipsInput,itineraryDay, anditineraryEntry, while removingitineraryItem.CheckboxFilterChipsInputwidget for multi-option selection.TravelIconenum andiconForutility function for consistent icon usage.ItineraryDaywidget to structure itinerary content by day.ItineraryEntrywidget for individual activities, transport, or accommodation within an itinerary day, including booking status.ItineraryDayandItineraryEntry.ItineraryDayandItineraryEntrycomponents and integratedDismissNotificationfor modal closure.common.dartforTravelIconandiconFor, and added aninitialValueproperty.Notificationclass for dismissing UI elements.CheckboxFilterChipsInput.ItineraryDay.ItineraryEntry.itinerary_item.dart.Viewing itinerary
Choosing a hotel
See itinerary updated with hotel
Helps to fix #260