Create data class CatalogItemContext and provide surfaceId to widgetBuilder. #491
Conversation
![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 CatalogItemContext data class to encapsulate the parameters passed to widgetBuilder functions. This is a great refactoring that simplifies the widgetBuilder signature, improving readability and making it easier to extend in the future. The changes have been applied consistently across all catalog items and their corresponding tests. Additionally, surfaceId is now provided to the widgetBuilder via this new context object. My review includes a few suggestions to further improve code clarity and adhere to Dart conventions, mainly regarding unused builder parameters and adding documentation for the new class.
| const SizedBox(height: 16.0), | ||
| ValueListenableBuilder<String?>( | ||
| valueListenable: notifier, | ||
| builder: (builderContext, submitLabel, child) { |
There was a problem hiding this comment.
The child parameter in the builder function is unused. According to Dart conventions, it's best to replace unused parameters with an underscore (_) to improve code clarity.
| builder: (builderContext, submitLabel, child) { | |
| builder: (builderContext, submitLabel, _) { |
| class CatalogItemContext { | ||
| CatalogItemContext({ | ||
| required this.data, | ||
| required this.id, | ||
| required this.buildChild, | ||
| required this.dispatchEvent, | ||
| required this.buildContext, | ||
| required this.dataContext, | ||
| required this.getComponent, | ||
| required this.surfaceId, | ||
| }); | ||
|
|
||
| final Object data; | ||
| final String id; | ||
| final ChildBuilderCallback buildChild; | ||
| final DispatchEventCallback dispatchEvent; | ||
| final BuildContext buildContext; | ||
| final DataContext dataContext; | ||
| final GetComponentCallback getComponent; | ||
| final String surfaceId; | ||
| } |
There was a problem hiding this comment.
It's good practice to add documentation comments to public classes and their members. The new CatalogItemContext class and its fields would benefit from doc comments explaining their purpose.
class CatalogItemContext {
/// Creates a context object for a catalog item widget builder.
CatalogItemContext({
required this.data,
required this.id,
required this.buildChild,
required this.dispatchEvent,
required this.buildContext,
required this.dataContext,
required this.getComponent,
required this.surfaceId,
});
/// The deserialized JSON data for this widget.
///
/// The format of this data will match [CatalogItem.dataSchema].
final Object data;
/// The unique ID of this widget instance.
final String id;
/// A function to build a child widget by its ID.
final ChildBuilderCallback buildChild;
/// A function to dispatch a UI event.
final DispatchEventCallback dispatchEvent;
/// The build context for the widget.
final BuildContext buildContext;
/// The current data context for this widget.
final DataContext dataContext;
/// A function to retrieve a component definition by its ID.
final GetComponentCallback getComponent;
/// The ID of the surface this widget belongs to.
final String surfaceId;
}| builder: (bcontext, label, child) { | ||
| return ValueListenableBuilder<bool?>( | ||
| valueListenable: valueNotifier, | ||
| builder: (bcontext, value, child) { | ||
| return CheckboxListTile( | ||
| title: Text(label ?? ''), | ||
| value: value ?? false, | ||
| onChanged: (newValue) { | ||
| final path = checkBoxData.value['path'] as String?; | ||
| if (path != null) { | ||
| context.dataContext.update(DataPath(path), newValue); | ||
| } | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
In both nested ValueListenableBuilders, some parameters of the builder functions are unused. It's a good practice to replace unused parameters with an underscore (_) for better readability.
- In the outer builder,
bcontextandchildare unused. - In the inner builder,
bcontextandchildare also unused.
| builder: (bcontext, label, child) { | |
| return ValueListenableBuilder<bool?>( | |
| valueListenable: valueNotifier, | |
| builder: (bcontext, value, child) { | |
| return CheckboxListTile( | |
| title: Text(label ?? ''), | |
| value: value ?? false, | |
| onChanged: (newValue) { | |
| final path = checkBoxData.value['path'] as String?; | |
| if (path != null) { | |
| context.dataContext.update(DataPath(path), newValue); | |
| } | |
| }, | |
| ); | |
| }, | |
| ); | |
| }, | |
| builder: (_, label, _) { | |
| return ValueListenableBuilder<bool?>( | |
| valueListenable: valueNotifier, | |
| builder: (_, value, _) { | |
| return CheckboxListTile( | |
| title: Text(label ?? ''), | |
| value: value ?? false, | |
| onChanged: (newValue) { | |
| final path = checkBoxData.value['path'] as String?; | |
| if (path != null) { | |
| context.dataContext.update(DataPath(path), newValue); | |
| } | |
| }, | |
| ); | |
| }, | |
| ); | |
| }, |
| .toList(), | ||
| ); | ||
| }, | ||
| templateListWidgetBuilder: (bcontext, list, componentId, dataBinding) { |
There was a problem hiding this comment.
The bcontext parameter in the templateListWidgetBuilder is unused. It's best to replace it with an underscore (_) to improve code clarity.
| templateListWidgetBuilder: (bcontext, list, componentId, dataBinding) { | |
| templateListWidgetBuilder: (_, list, componentId, dataBinding) { |
| builder: (bcontext, currentLocation, child) { | ||
| final location = currentLocation; | ||
| if (location == null || location.isEmpty) { | ||
| genUiLogger.warning( | ||
| 'Image widget created with no URL at path: ${context.dataContext.path}', | ||
| ); | ||
| return const SizedBox.shrink(); | ||
| } | ||
| final fit = imageData.fit; | ||
|
|
||
| late Widget child; | ||
| late Widget bchild; | ||
|
|
||
| if (location.startsWith('assets/')) { | ||
| child = Image.asset(location, fit: fit); | ||
| } else { | ||
| child = Image.network(location, fit: fit); | ||
| } | ||
| return SizedBox(width: 150, height: 150, child: child); | ||
| }, | ||
| ); | ||
| if (location.startsWith('assets/')) { | ||
| bchild = Image.asset(location, fit: fit); | ||
| } else { | ||
| bchild = Image.network(location, fit: fit); | ||
| } | ||
| return SizedBox(width: 150, height: 150, child: bchild); | ||
| }, |
There was a problem hiding this comment.
For better readability and to follow Dart conventions, it's recommended to handle unused parameters and variable naming more clearly.
- The
bcontextandchildparameters in thebuilderare unused and should be replaced with an underscore (_). - The variable name
bchildcan be confusing. A more descriptive name likeimageWidgetwould improve clarity.
| builder: (bcontext, currentLocation, child) { | |
| final location = currentLocation; | |
| if (location == null || location.isEmpty) { | |
| genUiLogger.warning( | |
| 'Image widget created with no URL at path: ${context.dataContext.path}', | |
| ); | |
| return const SizedBox.shrink(); | |
| } | |
| final fit = imageData.fit; | |
| late Widget child; | |
| late Widget bchild; | |
| if (location.startsWith('assets/')) { | |
| child = Image.asset(location, fit: fit); | |
| } else { | |
| child = Image.network(location, fit: fit); | |
| } | |
| return SizedBox(width: 150, height: 150, child: child); | |
| }, | |
| ); | |
| if (location.startsWith('assets/')) { | |
| bchild = Image.asset(location, fit: fit); | |
| } else { | |
| bchild = Image.network(location, fit: fit); | |
| } | |
| return SizedBox(width: 150, height: 150, child: bchild); | |
| }, | |
| builder: (_, currentLocation, _) { | |
| final location = currentLocation; | |
| if (location == null || location.isEmpty) { | |
| genUiLogger.warning( | |
| 'Image widget created with no URL at path: ${context.dataContext.path}', | |
| ); | |
| return const SizedBox.shrink(); | |
| } | |
| final fit = imageData.fit; | |
| late Widget imageWidget; | |
| if (location.startsWith('assets/')) { | |
| imageWidget = Image.asset(location, fit: fit); | |
| } else { | |
| imageWidget = Image.network(location, fit: fit); | |
| } | |
| return SizedBox(width: 150, height: 150, child: imageWidget); | |
| }, |
| ); | ||
| return ValueListenableBuilder<num?>( | ||
| valueListenable: valueNotifier, | ||
| builder: (_, value, child) { |
There was a problem hiding this comment.
The child parameter in the builder function is unused. According to Dart conventions, it's best to replace unused parameters with an underscore (_) to improve code clarity.
| builder: (_, value, child) { | |
| builder: (_, value, _) { |
| }) { | ||
| final checkboxFilterChipsData = _CheckboxFilterChipsInputData.fromMap( | ||
| data as Map<String, Object?>, | ||
| widgetBuilder: (context) { |
There was a problem hiding this comment.
Because context usually refers to a BuildContext, can we change the name of this to be "catalogItemContext" or something else that is specific so people reading this can tell it's not a BuildContext?
I think this will also help later where you have to come up with other names for the build context: you can go back to calling it context, and the catalog item one can be something else. Maybe itemContext?
There was a problem hiding this comment.
itemContext sounds good!
There was a problem hiding this comment.
I renamed it in main signature
will rename in other places in next PR
2 participants
It is follow up for #481
It is successor of #482
Ignoring gemini recommendations as I want to merge this massive change faster, to avoid merge conflicts.