-
Notifications
You must be signed in to change notification settings - Fork 56
Cleanup. #493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup. #493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a nice cleanup that refactors how widgets are built from the catalog. By introducing CatalogItemContext, the buildWidget method signature is simplified, and the parameters for widget building are neatly encapsulated. This improves code readability and maintainability. The consistent renaming of the context parameter to itemContext in all widget builders is also a great change that avoids potential confusion with Flutter's BuildContext.
I found one small area for further simplification in the catalog.dart file, which I've commented on. Overall, this is a solid improvement to the codebase.
| buildChild: (String childId, [DataContext? childDataContext]) => | ||
| buildChild(childId, childDataContext ?? dataContext), | ||
| dispatchEvent: dispatchEvent, | ||
| buildContext: context, | ||
| dataContext: dataContext, | ||
| getComponent: getComponent, | ||
| surfaceId: surfaceId, | ||
| itemContext.buildChild( | ||
| childId, | ||
| childDataContext ?? itemContext.dataContext, | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for wrapping itemContext.buildChild is redundant. The buildChild function provided in itemContext (which originates from _GenUiSurfaceState._buildWidget) already handles the case where childDataContext is null by defaulting to the parent's dataContext. You can simplify this by passing itemContext.buildChild directly, which makes the code cleaner and avoids an unnecessary closure.
| buildChild: (String childId, [DataContext? childDataContext]) => | |
| buildChild(childId, childDataContext ?? dataContext), | |
| dispatchEvent: dispatchEvent, | |
| buildContext: context, | |
| dataContext: dataContext, | |
| getComponent: getComponent, | |
| surfaceId: surfaceId, | |
| itemContext.buildChild( | |
| childId, | |
| childDataContext ?? itemContext.dataContext, | |
| ), | |
| buildChild: itemContext.buildChild, |
No description provided.