You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "nausharipov (via GitHub)" <gi...@apache.org> on 2023/05/10 11:05:59 UTC

[GitHub] [beam] nausharipov opened a new pull request, #26630: [Tour of Beam] [Frontend] Unit navigation draft

nausharipov opened a new pull request, #26630:
URL: https://github.com/apache/beam/pull/26630

   Previous, Next unit navigation.
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] 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)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](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] damccorm merged pull request #26630: [Tour of Beam] [Frontend] Unit navigation

Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm merged PR #26630:
URL: https://github.com/apache/beam/pull/26630


-- 
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 #26630: [Tour of Beam] [Frontend] Unit navigation draft

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26630:
URL: https://github.com/apache/beam/pull/26630#issuecomment-1541962498

   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] alexeyinkin commented on a diff in pull request #26630: [Tour of Beam] [Frontend] Unit navigation draft

Posted by "alexeyinkin (via GitHub)" <gi...@apache.org>.
alexeyinkin commented on code in PR #26630:
URL: https://github.com/apache/beam/pull/26630#discussion_r1191096910


##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -60,15 +63,20 @@ class ContentTreeController extends ChangeNotifier {
       _onParentNodePressed(node);
     } else if (node is UnitModel) {
       if (node != _currentNode) {
-        _currentNode = node;
+        _setUnit(node);
       }
     }
 
     if (_currentNode != null) {
-      _treeIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
+      _breadcrumbIds = _getNodeAncestors(_currentNode!, [_currentNode!.id]);
     }

Review Comment:
   Should this also go to `_setUnit()`?



##########
learning/tour-of-beam/frontend/lib/models/parent_node.dart:
##########
@@ -19,6 +19,7 @@
 import 'package:collection/collection.dart';
 
 import 'node.dart';
+import 'unit.dart';
 
 abstract class ParentNodeModel extends NodeModel {

Review Comment:
   ```suggestion
   abstract class ParentNodeModel<T extends NodeModel> extends NodeModel {
   ```



-- 
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 #26630: [Tour of Beam] [Frontend] Unit navigation draft

Posted by "alexeyinkin (via GitHub)" <gi...@apache.org>.
alexeyinkin commented on code in PR #26630:
URL: https://github.com/apache/beam/pull/26630#discussion_r1190821403


##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -29,25 +29,27 @@ import '../../../state.dart';
 
 class ContentTreeController extends ChangeNotifier {
   final Sdk initialSdk;
-  List<String> _treeIds;
+  List<String> _breadcrumbs;

Review Comment:
   ```suggestion
     List<String> _breadcrumbIds;
   ```



##########
learning/tour-of-beam/frontend/assets/translations/en.yaml:
##########
@@ -51,6 +51,8 @@ pages:
     solveYourself: Before revealing the solution, try solving the challenge on your own. Remember, the more you practice, the better you will become. Give it a shot and see how far you can get.
     example: Example
     myCode: My code
+    nextUnit: Next unit
+    previousUnit: Previous unit

Review Comment:
   ```suggestion
       nextUnit: Next Unit
       previousUnit: Previous Unit
   ```



##########
learning/tour-of-beam/frontend/lib/models/content_tree.dart:
##########
@@ -20,29 +20,50 @@ import '../repositories/models/get_content_tree_response.dart';
 import 'module.dart';
 import 'node.dart';
 import 'parent_node.dart';
+import 'unit.dart';
 
 class ContentTreeModel extends ParentNodeModel {
   final List<ModuleModel> modules;
 
-  String get sdkId => id;
+  final List<UnitModel> units;
 
-  @override
-  List<NodeModel> get nodes => modules;
+  String get sdkId => id;
 
   const ContentTreeModel({
     required super.id,
     required this.modules,
+    required this.units,
   }) : super(
           parent: null,
           title: '',
           nodes: modules,
         );
 
+  static List<UnitModel> _getUnitsFromModules(List<ModuleModel> modules) {
+    final units = <UnitModel>[];
+
+    void extractUnitsFromNode(NodeModel node) {
+      if (node is ParentNodeModel) {
+        node.nodes.forEach(extractUnitsFromNode);
+      } else if (node is UnitModel) {
+        units.add(node);
+      }
+    }
+
+    modules.forEach(extractUnitsFromNode);
+    return units;
+  }

Review Comment:
   To `ParentNodeModel.asList()`



##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/content_tree.dart:
##########
@@ -110,12 +112,44 @@ class ContentTreeController extends ChangeNotifier {
     }
 
     _toggleNode(
-      contentTree.getNodeByTreeIds(_treeIds) ?? contentTree.modules.first,
+      contentTree.getNodeByTreeIds(_breadcrumbs) ?? contentTree.modules.first,
     );
 
+    units.clear();
+    units.addAll(contentTree.getUnitsFromModules());
+
     notifyListeners();
   }
 
+  bool hasPreviousUnit() {
+    return _getCurrentUnitIndex() > 0;
+  }
+
+  bool hasNextUnit() {
+    return _getCurrentUnitIndex() < units.length - 1;
+  }
+
+  void openPreviousUnit() {
+    final previousUnit = units[_getCurrentUnitIndex() - 1];
+    _navigateToUnit(previousUnit);
+  }
+
+  void openNextUnit() {
+    final nextUnit = units[_getCurrentUnitIndex() + 1];
+    _navigateToUnit(nextUnit);
+  }
+
+  int _getCurrentUnitIndex() {

Review Comment:
   Cache the current index on any navigation or tree update.



##########
learning/tour-of-beam/frontend/lib/models/content_tree.dart:
##########
@@ -38,6 +36,21 @@ class ContentTreeModel extends ParentNodeModel {
           nodes: modules,
         );
 
+  List<UnitModel> getUnitsFromModules() {

Review Comment:
   To `ParentNodeModel.asList()`



##########
learning/tour-of-beam/frontend/lib/pages/tour/state.dart:
##########
@@ -60,7 +59,8 @@ class TourNotifier extends ChangeNotifier with PageStateMixin<void> {
     List<String> initialTreeIds = const [],
   })  : contentTreeController = ContentTreeController(
           initialSdk: initialSdk,
-          initialTreeIds: initialTreeIds,
+          // TODO(nausharipov) review: replace treeIds with breadcrumbs everywhere?

Review Comment:
   Yes.



-- 
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 #26630: [Tour of Beam] [Frontend] Unit navigation draft

Posted by "nausharipov (via GitHub)" <gi...@apache.org>.
nausharipov commented on PR #26630:
URL: https://github.com/apache/beam/pull/26630#issuecomment-1541959060

   R: @alexeyinkin 


-- 
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 #26630: [Tour of Beam] [Frontend] Unit navigation

Posted by "nausharipov (via GitHub)" <gi...@apache.org>.
nausharipov commented on PR #26630:
URL: https://github.com/apache/beam/pull/26630#issuecomment-1543960879

   R: @damccorm 


-- 
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 a diff in pull request #26630: [Tour of Beam] [Frontend] Unit navigation

Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #26630:
URL: https://github.com/apache/beam/pull/26630#discussion_r1192343270


##########
learning/tour-of-beam/frontend/integration_test/tour_page_test.dart:
##########
@@ -48,8 +48,9 @@ void main() {
       await _checkContentTreeBuildsProperly(wt);
       await _checkContentTreeScrollsProperly(wt);
       await _checkHighlightsSelectedUnit(wt);
-      await _checkRunCodeWorks(wt);
-      await _checkResizeUnitContent(wt);
+      // TODO(nausharipov): fix tests
+      // await _checkRunCodeWorks(wt);
+      // await _checkResizeUnitContent(wt);

Review Comment:
   Why do we need to disable these (and test checks elsewhere)? Is it related to the changes in this PR?



-- 
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 a diff in pull request #26630: [Tour of Beam] [Frontend] Unit navigation

Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #26630:
URL: https://github.com/apache/beam/pull/26630#discussion_r1192346470


##########
learning/tour-of-beam/frontend/integration_test/tour_page_test.dart:
##########
@@ -48,8 +48,9 @@ void main() {
       await _checkContentTreeBuildsProperly(wt);
       await _checkContentTreeScrollsProperly(wt);
       await _checkHighlightsSelectedUnit(wt);
-      await _checkRunCodeWorks(wt);
-      await _checkResizeUnitContent(wt);
+      // TODO(nausharipov): fix tests
+      // await _checkRunCodeWorks(wt);
+      // await _checkResizeUnitContent(wt);

Review Comment:
   Also, looks like CI is still failing:
   
   ```
   Failure in method: ToB miscellaneous ui
   ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞═════════════════
   The following ExampleLoadingException was thrown running a test:
   Cannot load the example.
   ```



-- 
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 #26630: [Tour of Beam] [Frontend] Unit navigation

Posted by "nausharipov (via GitHub)" <gi...@apache.org>.
nausharipov commented on code in PR #26630:
URL: https://github.com/apache/beam/pull/26630#discussion_r1192350577


##########
learning/tour-of-beam/frontend/integration_test/tour_page_test.dart:
##########
@@ -48,8 +48,9 @@ void main() {
       await _checkContentTreeBuildsProperly(wt);
       await _checkContentTreeScrollsProperly(wt);
       await _checkHighlightsSelectedUnit(wt);
-      await _checkRunCodeWorks(wt);
-      await _checkResizeUnitContent(wt);
+      // TODO(nausharipov): fix tests
+      // await _checkRunCodeWorks(wt);
+      // await _checkResizeUnitContent(wt);

Review Comment:
   @damccorm the test failures are not related to the changes in this PR. @Malarg plans to fix them in a separate PR.



-- 
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 a diff in pull request #26630: [Tour of Beam] [Frontend] Unit navigation

Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on code in PR #26630:
URL: https://github.com/apache/beam/pull/26630#discussion_r1192365051


##########
learning/tour-of-beam/frontend/integration_test/tour_page_test.dart:
##########
@@ -48,8 +48,9 @@ void main() {
       await _checkContentTreeBuildsProperly(wt);
       await _checkContentTreeScrollsProperly(wt);
       await _checkHighlightsSelectedUnit(wt);
-      await _checkRunCodeWorks(wt);
-      await _checkResizeUnitContent(wt);
+      // TODO(nausharipov): fix tests
+      // await _checkRunCodeWorks(wt);
+      // await _checkResizeUnitContent(wt);

Review Comment:
   SGTM



-- 
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