You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/10/21 12:48:11 UTC
[GitHub] [beam] nausharipov opened a new pull request, #23776: [Tour of Beam] [Frontend] Unit content URLs
nausharipov opened a new pull request, #23776:
URL: https://github.com/apache/beam/pull/23776
Resolves #23593
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on PR #23776:
URL: https://github.com/apache/beam/pull/23776#issuecomment-1311278482
LGTM (internal review).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1017097436
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,88 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
Review Comment:
Then state is not updated when needed.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1017097122
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -36,23 +36,42 @@ class GroupWidget extends StatelessWidget {
@override
Widget build(BuildContext context) {
- return ExpansionTileWrapper(
- ExpansionTile(
- tilePadding: EdgeInsets.zero,
- title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
- ),
- childrenPadding: const EdgeInsets.only(
- left: BeamSizes.size24,
- ),
- children: [
- GroupNodesWidget(
- nodes: group.nodes,
- contentTreeController: contentTreeController,
+ return AnimatedBuilder(
+ animation: contentTreeController,
+ builder: (context, child) {
+ final isExpanded = contentTreeController.expandedIds.contains(group.id);
+
+ return ExpansionTileWrapper(
+ ExpansionTile(
Review Comment:
Does it need to contain all of `ExpansionTile` content?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1014602313
##########
playground/frontend/playground_components/lib/src/constants/colors.dart:
##########
@@ -36,45 +36,51 @@ class BeamColors {
class BeamGraphColors {
static const node = BeamColors.grey3;
- static const border = Color(0xFF45454E);
+ static const border = Color(0xff45454E);
Review Comment:
It is for a visual separation of alpha and color code.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm commented on pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #23776:
URL: https://github.com/apache/beam/pull/23776#issuecomment-1317296004
Looks like there are conflicts that need to be resolved before merging - @nausharipov could you please take a look?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1014601708
##########
playground/frontend/playground_components/lib/src/theme/theme.dart:
##########
@@ -32,6 +32,9 @@ class BeamThemeExtension extends ThemeExtension<BeamThemeExtension> {
final Color primaryBackgroundTextColor;
final Color lightGreyBackgroundTextColor;
final Color secondaryBackgroundColor;
+ // TODO(nausharipov): simplify new color addition
+ final Color selectedProgressColor;
+ final Color unselectedProgressColor;
Review Comment:
The selected progress indicator is darker in light mode and lighter in dark mode compared to the unselected state. AFAIK, a semi-transparent indicator would look differently.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on PR #23776:
URL: https://github.com/apache/beam/pull/23776#issuecomment-1311285764
R: @damondouglas
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1019491637
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/stateless_expansion_tile.dart:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../../components/expansion_tile_wrapper.dart';
+
+class StatelessExpansionTile extends StatelessWidget {
+ final bool isExpanded;
+ final void Function(bool)? onExpansionChanged;
Review Comment:
`final ValueChanged<bool>? onExpansionChanged;`
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/stateless_expansion_tile.dart:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../../components/expansion_tile_wrapper.dart';
+
+class StatelessExpansionTile extends StatelessWidget {
+ final bool isExpanded;
+ final void Function(bool)? onExpansionChanged;
+ final Widget title;
+ final Widget child;
+
+ const StatelessExpansionTile({
+ required super.key,
+ required this.isExpanded,
+ required this.onExpansionChanged,
+ required this.title,
+ required this.child,
+ });
+
+ @override
+ Widget build(BuildContext context) {
+ return ExpansionTileWrapper(
+ ExpansionTile(
+ // TODO(nausharipov): isn't rebuilt without key.
+ key: key,
Review Comment:
`key: ValueKey(isExpanded)`
Then there may be a case when we still need the key in this widget. Then we will invent a combination like `ValueKey('$key-$isExpanded')` or something like that.
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/stateless_expansion_tile.dart:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:flutter/material.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../../components/expansion_tile_wrapper.dart';
+
+class StatelessExpansionTile extends StatelessWidget {
+ final bool isExpanded;
+ final void Function(bool)? onExpansionChanged;
+ final Widget title;
+ final Widget child;
+
+ const StatelessExpansionTile({
+ required super.key,
Review Comment:
Delete this.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] github-actions[bot] commented on pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23776:
URL: https://github.com/apache/beam/pull/23776#issuecomment-1311286421
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1014599799
##########
learning/tour-of-beam/frontend/lib/models/content_tree.dart:
##########
@@ -18,15 +18,25 @@
import '../repositories/models/get_content_tree_response.dart';
import 'module.dart';
+import 'node.dart';
+import 'parent_node.dart';
-class ContentTreeModel {
+class ContentTreeModel extends ParentNodeModel {
final String sdkId;
Review Comment:
There's a warning (Don't override fields) if a field is named `id`. Is `id` better be renamed to something else like `sdk`? Or is the parent `id` supposed to be used?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1014602034
##########
learning/tour-of-beam/frontend/pubspec.lock:
##########
@@ -278,7 +278,7 @@ packages:
name: flutter_code_editor
url: "https://pub.dartlang.org"
source: hosted
- version: "0.1.1"
+ version: "0.1.3"
Review Comment:
I rebased the branch on top of `master`.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nausharipov commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
nausharipov commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1017097122
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -36,23 +36,42 @@ class GroupWidget extends StatelessWidget {
@override
Widget build(BuildContext context) {
- return ExpansionTileWrapper(
- ExpansionTile(
- tilePadding: EdgeInsets.zero,
- title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
- ),
- childrenPadding: const EdgeInsets.only(
- left: BeamSizes.size24,
- ),
- children: [
- GroupNodesWidget(
- nodes: group.nodes,
- contentTreeController: contentTreeController,
+ return AnimatedBuilder(
+ animation: contentTreeController,
+ builder: (context, child) {
+ final isExpanded = contentTreeController.expandedIds.contains(group.id);
+
+ return ExpansionTileWrapper(
+ ExpansionTile(
Review Comment:
Does it need to contain all of `ExpansionTile` content?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1013884620
##########
learning/tour-of-beam/frontend/lib/models/content_tree.dart:
##########
@@ -18,15 +18,25 @@
import '../repositories/models/get_content_tree_response.dart';
import 'module.dart';
+import 'node.dart';
+import 'parent_node.dart';
-class ContentTreeModel {
+class ContentTreeModel extends ParentNodeModel {
final String sdkId;
Review Comment:
`String get sdkId => id;` ?
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,82 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
+ notifyListeners();
+ }
+
+ List<String> _getNodeAncestors(NodeModel node, List<String> ancestors) {
+ if (node.parent != null) {
+ ancestors.add(node.parent!.id);
+ return _getNodeAncestors(node.parent!, ancestors);
Review Comment:
`[...ancestorIds, node.parent!.id]`
We should avoid adding to passed lists. The reasons are:
1. The list can be used in the calling code.
2. The list may be unmodifiable.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,82 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
+ notifyListeners();
+ }
+
+ List<String> _getNodeAncestors(NodeModel node, List<String> ancestors) {
Review Comment:
`ancestorIds`
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/unit.dart:
##########
@@ -33,16 +33,48 @@ class UnitWidget extends StatelessWidget {
required this.contentTreeController,
});
+ @override
+ State<UnitWidget> createState() => _UnitWidgetState();
+}
+
+class _UnitWidgetState extends State<UnitWidget> {
+ @override
+ void initState() {
+ super.initState();
+ widget.contentTreeController.addListener(_rebuild);
+ }
+
+ @override
+ void dispose() {
+ widget.contentTreeController.removeListener(_rebuild);
+ super.dispose();
+ }
+
+ void _rebuild() {
+ setState(() {});
Review Comment:
Use `AnimatedBuilder` instead.
##########
playground/frontend/playground_components/lib/src/constants/colors.dart:
##########
@@ -36,45 +36,51 @@ class BeamColors {
class BeamGraphColors {
static const node = BeamColors.grey3;
- static const border = Color(0xFF45454E);
+ static const border = Color(0xff45454E);
Review Comment:
Is this deliberate? Why using lowercase for alpha and uppercase for others?
##########
learning/tour-of-beam/frontend/pubspec.lock:
##########
@@ -278,7 +278,7 @@ packages:
name: flutter_code_editor
url: "https://pub.dartlang.org"
source: hosted
- version: "0.1.1"
+ version: "0.1.3"
Review Comment:
It's already `0.1.3` in `master`. This will likely cause a merge conflict.
##########
playground/frontend/playground_components/lib/src/theme/theme.dart:
##########
@@ -32,6 +32,9 @@ class BeamThemeExtension extends ThemeExtension<BeamThemeExtension> {
final Color primaryBackgroundTextColor;
final Color lightGreyBackgroundTextColor;
final Color secondaryBackgroundColor;
+ // TODO(nausharipov): simplify new color addition
+ final Color selectedProgressColor;
+ final Color unselectedProgressColor;
Review Comment:
Can these use a single color with some transparency?
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -34,22 +34,44 @@ class GroupWidget extends StatelessWidget {
required this.contentTreeController,
});
+ @override
+ State<GroupWidget> createState() => _GroupWidgetState();
+}
+
+class _GroupWidgetState extends State<GroupWidget> {
@override
Widget build(BuildContext context) {
+ final isExpanded =
+ widget.contentTreeController.expandedIds.contains(widget.group.id);
+
return ExpansionTileWrapper(
ExpansionTile(
+ key: Key('${widget.group.id}$isExpanded'),
+ initiallyExpanded: isExpanded,
tilePadding: EdgeInsets.zero,
+ onExpansionChanged: (expand) {
+ /// Since expandedIds is also used for expansion, it needs to be
+ /// updated to prevent the tile to stay collapsed on title tap.
+ if (!expand) {
+ widget.contentTreeController.expandedIds.remove(widget.group.id);
+ setState(() {});
+ }
+ },
title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
+ group: widget.group,
+ onTap: () {
+ widget.contentTreeController.openNode(widget.group);
+ // Couldn't make it rebuild reliably with AnimatedBuilder
Review Comment:
Put such comments in a form of TODO.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,82 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
+ notifyListeners();
+ }
+
+ List<String> _getNodeAncestors(NodeModel node, List<String> ancestors) {
+ if (node.parent != null) {
+ ancestors.add(node.parent!.id);
+ return _getNodeAncestors(node.parent!, ancestors);
+ } else {
Review Comment:
No `else` after `return`.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1015084877
##########
learning/tour-of-beam/frontend/lib/models/content_tree.dart:
##########
@@ -18,15 +18,25 @@
import '../repositories/models/get_content_tree_response.dart';
import 'module.dart';
+import 'node.dart';
+import 'parent_node.dart';
-class ContentTreeModel {
+class ContentTreeModel extends ParentNodeModel {
final String sdkId;
Review Comment:
I mean `sdkId` getter that returns super's `id`.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] alexeyinkin commented on a diff in pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
alexeyinkin commented on code in PR #23776:
URL: https://github.com/apache/beam/pull/23776#discussion_r1015091568
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,88 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
Review Comment:
`return` after this line would cut redundant `notifyListeners()` calls.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,88 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
ContentTreeController({
required String initialSdkId,
List<String> initialTreeIds = const [],
}) : _sdkId = initialSdkId,
- _treeIds = initialTreeIds;
+ _treeIds = initialTreeIds {
+ expandedIds.addAll(initialTreeIds);
+ _contentTreeCache.addListener(_onContentTreeCacheChange);
+ _onContentTreeCacheChange();
+ }
Sdk get sdk => Sdk.parseOrCreate(_sdkId);
String get sdkId => _sdkId;
List<String> get treeIds => _treeIds;
NodeModel? get currentNode => _currentNode;
- void onNodeTap(NodeModel node) {
+ void openNode(NodeModel node) {
+ if (!expandedIds.contains(node.id)) {
+ expandedIds.add(node.id);
+ }
+
if (node == _currentNode) {
return;
}
- _currentNode = node;
- // TODO(alexeyinkin): Set _treeIds from node.
+ if (node is GroupModel) {
+ openNode(node.nodes.first);
+ } else if (node is UnitModel) {
+ _currentNode = node;
+ }
+
+ if (_currentNode != null) {
+ _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+ }
notifyListeners();
}
+
+ void markGroupAsCollapsed(GroupModel group) {
Review Comment:
`collapseGroup`?
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -36,23 +36,42 @@ class GroupWidget extends StatelessWidget {
@override
Widget build(BuildContext context) {
- return ExpansionTileWrapper(
- ExpansionTile(
- tilePadding: EdgeInsets.zero,
- title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
- ),
- childrenPadding: const EdgeInsets.only(
- left: BeamSizes.size24,
- ),
- children: [
- GroupNodesWidget(
- nodes: group.nodes,
- contentTreeController: contentTreeController,
+ return AnimatedBuilder(
+ animation: contentTreeController,
+ 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.
Review Comment:
This comment feels redundant.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -17,33 +17,88 @@
*/
import 'package:flutter/widgets.dart';
+import 'package:get_it/get_it.dart';
import 'package:playground_components/playground_components.dart';
+import '../../../cache/content_tree.dart';
+import '../../../models/group.dart';
import '../../../models/node.dart';
+import '../../../models/unit.dart';
class ContentTreeController extends ChangeNotifier {
String _sdkId;
List<String> _treeIds;
NodeModel? _currentNode;
+ final _contentTreeCache = GetIt.instance.get<ContentTreeCache>();
+ final expandedIds = <String>{};
Review Comment:
This is better made private + getter.
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/tour_progress_indicator.dart:
##########
@@ -32,7 +37,16 @@ class TourProgressIndicator extends StatelessWidget {
left: BeamSizes.size4,
right: BeamSizes.size8,
),
- child: SvgPicture.asset(assetPath),
+ child: SvgPicture.asset(
+ assetPath,
+ color: isSelected
+ ? Theme.of(context)
+ .extension<BeamThemeExtension>()!
+ .selectedProgressColor
+ : Theme.of(context)
+ .extension<BeamThemeExtension>()!
+ .unselectedProgressColor,
Review Comment:
If `Theme.of(context).extension...` is repeated, it's better to grab it early like `final ext = Theme.of(context).extension...`
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/unit.dart:
##########
@@ -35,17 +35,34 @@ class UnitWidget extends StatelessWidget {
@override
Widget build(BuildContext context) {
- return ClickableWidget(
- onTap: () => contentTreeController.onNodeTap(unit),
- child: Padding(
- padding: const EdgeInsets.symmetric(vertical: BeamSizes.size10),
- child: Row(
- children: [
- TourProgressIndicator(assetPath: Assets.svg.unitProgress0),
- Expanded(child: Text(unit.title)),
- ],
- ),
- ),
+ return AnimatedBuilder(
+ animation: contentTreeController,
+ builder: (context, child) {
+ final bool isSelected =
Review Comment:
No `bool` here.
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -36,23 +36,42 @@ class GroupWidget extends StatelessWidget {
@override
Widget build(BuildContext context) {
- return ExpansionTileWrapper(
- ExpansionTile(
- tilePadding: EdgeInsets.zero,
- title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
- ),
- childrenPadding: const EdgeInsets.only(
- left: BeamSizes.size24,
- ),
- children: [
- GroupNodesWidget(
- nodes: group.nodes,
- contentTreeController: contentTreeController,
+ return AnimatedBuilder(
+ animation: contentTreeController,
+ 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);
+ }
Review Comment:
What about the opposite case? If the tile is expanded with an icon tap, how is `expandedIds` updated?
##########
learning/tour-of-beam/frontend/lib/pages/tour/widgets/group.dart:
##########
@@ -36,23 +36,42 @@ class GroupWidget extends StatelessWidget {
@override
Widget build(BuildContext context) {
- return ExpansionTileWrapper(
- ExpansionTile(
- tilePadding: EdgeInsets.zero,
- title: GroupTitleWidget(
- group: group,
- onTap: () => contentTreeController.onNodeTap(group),
- ),
- childrenPadding: const EdgeInsets.only(
- left: BeamSizes.size24,
- ),
- children: [
- GroupNodesWidget(
- nodes: group.nodes,
- contentTreeController: contentTreeController,
+ return AnimatedBuilder(
+ animation: contentTreeController,
+ builder: (context, child) {
+ final isExpanded = contentTreeController.expandedIds.contains(group.id);
+
+ return ExpansionTileWrapper(
+ ExpansionTile(
Review Comment:
This can be encapsulated in `StatelessExpanstionTile` widget. It should take `bool isExpanded` and only be guided by it and not its internal state.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damccorm merged pull request #23776: [Tour of Beam] [Frontend] Content tree URLs
Posted by GitBox <gi...@apache.org>.
damccorm merged PR #23776:
URL: https://github.com/apache/beam/pull/23776
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org