feat: enable nesting named page router outlets by ADjenkov · Pull Request #1556 · NativeScript/nativescript-angular

ADjenkov · 2018-10-15T15:27:35Z

TO DO: Add description
Resolve: #1351

ADjenkov · 2018-10-18T10:28:16Z

test branch_testsappng#djenkov/nested-outlets sdk_ng

vakrilov · 2018-10-19T08:51:30Z

@@ -342,10 +361,27 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire
    }

    private markActivatedRoute(activatedRoute: ActivatedRoute) {


Add comments on what this method does

vakrilov · 2018-10-19T10:36:09Z

        private parentContexts: ChildrenOutletContexts,
        private location: ViewContainerRef,
        @Attribute("name") name: string,
+        @Attribute("isNSEmptyOutlet") isNSEmptyOutlet: boolean,


The property can be named isEmptyOutlet -> it is already inside a PageRouterOutlet class that is NS-specific.
Also you can inject and define it in the same time

Suggested change

@Attribute("isNSEmptyOutlet") isNSEmptyOutlet: boolean,

@Attribute("isNSEmptyOutlet") private isEmptyOutlet: boolean,

manoldonev · 2018-10-22T10:57:12Z

        const resolver = componentContainer.injector.get(ComponentFactoryResolver);

-        const frame = parentView.page && parentView.page.frame;
+        let frame = parentView;


Can we add a comment about the scenario where parentView is a Frame? (it seems previously there wasn't such scenario?)

manoldonev · 2018-10-22T10:57:28Z

        const resolver = componentContainer.injector.get(ComponentFactoryResolver);

-        const frame = parentView.page && parentView.page.frame;
+        let frame = parentView;


Can we add a comment about the scenario where parentView is a Frame? (it seems previously there wasn't such scenario?)

manoldonev · 2018-10-22T12:25:29Z

                Object.keys(rootOutlets).forEach(outletName => {
-                    const topState = this.peekState(outletName);
+                    const outlet = this.findOutletByKey(outletName);
+                    const topState = outlet.peekState();


topState is not guarded for null

manoldonev · 2018-10-22T12:26:27Z

+        } else {
+            const queue = [];
+            queue.push(urlTreeRoot);
+            let currentTree = queue.shift();


const queue = []; let currentTree = urlTreeRoot;

manoldonev · 2018-10-22T12:27:02Z

+    // tslint:disable-next-line:component-selector
+    selector: "ns-empty-outlet",
+    moduleId: module.id,
+    template: "<page-router-outlet isNSEmptyOutlet='true'></page-router-outlet>"


If isNSEmptyOutlet is renamed this should be renamed as well.

manoldonev · 2018-10-22T12:27:47Z

        const resolver = componentContainer.injector.get(ComponentFactoryResolver);

-        const frame = parentView.page && parentView.page.frame;
+        let frame = parentView;


Can we add a comment about the scenario where parentView is a Frame? (it seems previously there wasn't such scenario?)

manoldonev · 2018-10-22T12:28:34Z

        }

-        const state = this.peekState(this.currentOutlet);
+        const state = this.currentOutlet && this.currentOutlet.peekState();


Is it possible for this.currentOutlet to be null when this.currentUrlTree is not null?

We set currentUrlTree inside pushStateInternal where all incoming urls are transformed into outlets . There will be always at least 1 Outlet set as currentOutlet . The otherwise should be possible only if pushState() is called from Angular with empty url param.

manoldonev · 2018-10-22T12:30:52Z

+        this.currentUrlTree = urlSerializer.parse(url);
+        const urlTreeRoot = this.currentUrlTree.root;
+
+        if (!Object.keys(urlTreeRoot.children).length) {


To reduce indentation of the main case instead of:

if (!Object.keys(urlTreeRoot.children).length) { } else { // else clause statements go here... }

Use:

if (!Object.keys(urlTreeRoot.children).length) { // ... return; } // else clause statements go here...

manoldonev · 2018-10-22T12:32:09Z

-        } else {
-            tree.root.children[this.currentOutlet] = state.segmentGroup;
+        } else if (changedOutlet) {
+            // tslint:disable-next-line:max-line-length


Probably comment is unnecessary at this point.

manoldonev · 2018-10-22T12:58:13Z

-                this.location._beginCloseModalNavigation();
                componentView.closeModal();
-                this.location.back();
                this.location._finishCloseModalNavigation();


Maybe rename to _closeModalNavigation?

manoldonev · 2018-10-22T13:15:13Z

+        return null;
+    }
+
+    containsLastState(stateUrl: string): boolean {


Maybe containsTopState?

manoldonev · 2018-10-22T13:15:23Z

+    isPageNavigationBack: boolean;
+    outletKeys: Array<string>;
+    pathByOutlets: string;
+    statesByOutlet: Array<LocationState> = [];


Rename to states?

manoldonev · 2018-10-22T13:41:39Z

+                segmentGroup = segmentGroup.children[currentPath];
+            } else {
+                segmentGroup = null;
+                break;


Explain this with a comment

SvetoslavTsenov · 2018-10-22T14:43:01Z

test nested-router-tab-view#djenkov/nested-outlets

manoldonev · 2018-10-22T14:54:31Z

+    modalNavigationDepth: number;
+    parent: Outlet;
+    isPageNavigationBack: boolean;
+    outletKeys: Array<string>;


[Reminder] As discussed it would be useful to add a comment why Outlet can have multiple (two) keys.

manoldonev · 2018-10-22T15:38:37Z

+                        }
+                    }
+
+                    queue.push(currentTree.children[outletName]);


[Reminder] queue.push(currentSegmentGroup) for clarity.

manoldonev · 2018-10-22T15:48:12Z

+
+        while (segmentGroup) {
+            const url = segmentGroup.toString();
+            fullPath = fullPath ? (url ? url + "/" : url) + fullPath : url;


This one-liner is concise but that also makes it unnecessarily complex to understand.

manoldonev · 2018-10-22T15:54:16Z

+    private updateSegmentGroup(rootNode, oldSegmentGroup: UrlSegmentGroup, newSegmentGroup: UrlSegmentGroup) {
+        const queue = [];
+        queue.push(rootNode);
+        let currentTree = queue.shift();


const queue = []; let currentTree = rootNode;

SvetoslavTsenov · 2018-10-22T20:39:41Z

test branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

feat(location-strategy): refactor location strategy to support nested p-r-o feat(reuse-strategy): refactor reuse strategy to support nested p-r-o chore(frame-service): cleanup FrameService from unused methods feat(router-extensions): extend back() to handle specific outlets by relative route feat(location-strategy): rework location strategy to support simultaneously outlets back navigation chore(e2e-tests): extend nested tabs test app with 'split view' chore: mark states with the current parent route path chore: add parent outlet to Outlet chore(router-extensions): prevent back navigation of outlet that already navigating back chore(reuse-strategy): extend cache key with parent route state string (unique) fix(router-extensions): fix back navigation when only relative route provided. Should find and back in the current outlet chore(e2e-tests): extend nested-router-tab-view app with tests chore(location-strategy): rework location strategy to store outlets by their parent path and current outlet name chore(reuse-strategy): child components should not be reteached when parent is detached chore(e2e-tests): extend e2e tests chore(reuse-strategy): modify shouldDetach to mark parent outlet chore(location-strategy): Modify getRouteFullPath method chore(p-r-o): mark all activated routes child routes that have coresponding outlet chore(router-extensions): find top activated route node for outlet when going back chore(location-strategy): modify getRouteFullPath to generates proper url path chore(p-r-o): find the top activated route for outlet inside activateWith method chore(e2e-tests): extend nested tab view app with lazy loaded home module feat(router-extensions): extend canGoBack() to handle specific outlets by relative route e2e tests added feat(modal-views): remove the back navigation when destroying modal views with p-r-o Introduce modalNavigationDepth to mark all p-r-o , along with their states, shown as modals. chore(e2e-modal-navigation): modify goBack to back in the current ActivatedRoute feat(modal-views): create outlet for any p-r-o displayed as modal view (even if it's 'primary') feat: introduce NSEmptyOutletComponent to support nested named lazy loaded modules feat(page-router-outlet): support named lazy loaded outlet Allow Outlet to be associated with more than one p-r-o (NSEmptyOutlet)

ADjenkov · 2018-10-23T10:48:28Z

test nativescript_ng_cosmos branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

ADjenkov · 2018-10-23T14:23:25Z

test nativescript_ng_cosmos branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

ADjenkov · 2018-10-23T14:41:48Z

test nativescript_ng_cosmos branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

SvetoslavTsenov · 2018-10-23T15:33:01Z

test nativescript_ng_cosmos branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets nativescript_ng_cosmos_api28 nativescript_ng_cosmos_ios12

SvetoslavTsenov · 2018-10-23T19:09:35Z

test nativescript_ng_cosmos branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets nativescript_ng_cosmos_api28 nativescript_ng_cosmos_ios12

SvetoslavTsenov · 2018-10-23T19:09:51Z

test branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

SvetoslavTsenov · 2018-10-23T20:53:12Z

test branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

SvetoslavTsenov · 2018-10-24T15:31:53Z

test branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

SvetoslavTsenov · 2018-10-24T19:48:24Z

test branch_nested-router-tab-view#djenkov/nested-outlets testsappng#djenkov/nested-outlets

ADjenkov added feature docs needed labels Oct 15, 2018

ADjenkov self-assigned this Oct 15, 2018

ghost added the in progress label Oct 15, 2018

ADjenkov force-pushed the djenkov/nested-outlets branch from c840df2 to af4a15d Compare October 17, 2018 11:10

MartoYankov mentioned this pull request Oct 17, 2018

feat(modals): easier ViewContainerRef handling #1546

Open

ADjenkov force-pushed the djenkov/nested-outlets branch 2 times, most recently from d3250ec to 7d68733 Compare October 18, 2018 10:27

vakrilov reviewed Oct 19, 2018

View reviewed changes

manoldonev reviewed Oct 22, 2018

View reviewed changes

ghost assigned SvetoslavTsenov Oct 22, 2018

ADjenkov and others added 6 commits October 23, 2018 13:45

chore(router-e2e): extend with lazy loaded named outlet

1095409

chore(location-strategy): extend replaceState methot

2a3dac4

chore(unit-tests): modify unit tests to the new location strategy

df1bfbd

chore: update bump modules and platform versoins

7049dfb

chore(location-strategy): revert replaceState method

37f9aa4

ADjenkov force-pushed the djenkov/nested-outlets branch from b9329d2 to 37f9aa4 Compare October 23, 2018 10:48

ADjenkov mentioned this pull request Oct 23, 2018

Align to new location strategy API NativeScript/tests-app-ng#39

Merged

chore: repsect PR comments

7259fc1

ADjenkov force-pushed the djenkov/nested-outlets branch from 452cb8f to 7259fc1 Compare October 23, 2018 14:41

chore: update e2e tests

b37b35d

chore: revert buttons

ecad0d2

tests: add actionBarVisibility property

35437e4

SvetoslavTsenov merged commit 46a0dc0 into master Oct 25, 2018

ghost removed feature in progress labels Oct 25, 2018

SvetoslavTsenov deleted the djenkov/nested-outlets branch October 25, 2018 07:16

dtopuzov mentioned this pull request Oct 29, 2018

Livesync with bundle and uglify do not work #1572

Closed

		@@ -342,10 +361,27 @@ export class PageRouterOutlet implements OnDestroy { // tslint:disable-line:dire
		}

		private markActivatedRoute(activatedRoute: ActivatedRoute) {

	@Attribute("isNSEmptyOutlet") isNSEmptyOutlet: boolean,
	@Attribute("isNSEmptyOutlet") private isEmptyOutlet: boolean,

Uh oh!

Conversation

ADjenkov commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ADjenkov commented Oct 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vakrilov Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvetoslavTsenov commented Oct 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SvetoslavTsenov commented Oct 22, 2018

Uh oh!

ADjenkov commented Oct 23, 2018

Uh oh!

ADjenkov commented Oct 23, 2018

Uh oh!

ADjenkov commented Oct 23, 2018

Uh oh!

SvetoslavTsenov commented Oct 23, 2018

Uh oh!

SvetoslavTsenov commented Oct 23, 2018

Uh oh!

SvetoslavTsenov commented Oct 23, 2018

Uh oh!

SvetoslavTsenov commented Oct 23, 2018

Uh oh!

SvetoslavTsenov commented Oct 24, 2018

Uh oh!

SvetoslavTsenov commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ADjenkov commented Oct 15, 2018 •

edited

Loading

vakrilov Oct 19, 2018 •

edited

Loading