alexeyinkin commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1017563305
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -68,8 +71,12 @@ class ContentTreeController extends ChangeNotifier {
notifyListeners();
}
- void markGroupAsCollapsed(GroupModel group) {
- expandedIds.remove(group.id);
+ void updateExpandedIds(GroupModel group, {required bool isExpanding}) {
Review Comment:
Different behavior based on a boolean flag is an anti-pattern. This should
be two separate methods: `expandGroup`, `collapseGroup`.
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -41,37 +41,56 @@ class GroupWidget extends StatelessWidget {
builder: (context, child) {
final isExpanded =
contentTreeController.expandedIds.contains(group.id);
- return ExpansionTileWrapper(
- ExpansionTile(
- key: Key('${group.id}$isExpanded'),
- initiallyExpanded: isExpanded,
- tilePadding: EdgeInsets.zero,
- onExpansionChanged: (isExpanding) {
- /// Since expandedIds is also used for expansion,
- /// it needs to be updated to prevent the tile from
- /// remaining collapsed after taps on title.
- if (!isExpanding) {
- contentTreeController.markGroupAsCollapsed(group);
- }
- },
- title: GroupTitleWidget(
- group: group,
- onTap: () {
- contentTreeController.openNode(group);
- },
- ),
- childrenPadding: const EdgeInsets.only(
- left: BeamSizes.size24,
- ),
- children: [
- GroupNodesWidget(
- nodes: group.nodes,
- contentTreeController: contentTreeController,
- ),
- ],
- ),
+ return _StatelessExpansionTile(
+ contentTreeController: contentTreeController,
+ group: group,
+ isExpanded: isExpanded,
);
},
);
}
}
+
+class _StatelessExpansionTile extends StatelessWidget {
Review Comment:
I had an idea of a generic `StatelessExpensionTile` that has no idea of
group and controller, and would use most of `ExpansionTile` parameters except
that it is guided by an external boolean state. It could then be reusable. Not
that we plan to reuse it soon, but its use is just cleaner because then we
would not have `key`.
This change adds an ad-hoc widget to the extent that it is private. In this
context it feels more of a cluttering than a benefit. For instance, it has
`isExpanded` which is a function of its other two parameters.
I suggest either reverting this change, or going the full way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
