enabled arrow icons to jump to previous and next lessons by sunilj74 · Pull Request #1041 · codelab-fun/codelab

sunilj74 · 2019-09-13T15:10:25Z

No description provided.

kirjs · 2019-09-14T15:57:36Z

apps/codelab/src/app/codelabs/angular/create-first-app/create-first-app.component.html

 <slide-deck slideShortcuts slidesRouting slides-tracking>
  <codelab-progress-bar></codelab-progress-bar>
-  <slide-arrows></slide-arrows>
+  <slide-arrows previousLink="" nextLink="/angular/templates"></slide-arrows>


We definitely want to avoid providing links manually

kirjs · 2019-09-14T17:26:27Z

Discussed offline, let's move to the deck component and populate using slides rather than inputs

kirjs · 2019-09-14T17:26:32Z

Discussed offline, let's move to the deck component and populate using slides rather than inputs

… the first and last slides

…into lesson_1024

…slide

NothingEverHappens · 2019-09-28T07:29:14Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts


-  constructor() {}
+  constructor(
+    @Optional() private readonly presentation: SlidesDeckComponent,


Add a comment on why it's optional?
I'm actually curious what the use case would be, since it's always supposed to be inside of the slide deck

ng test was failing so made it optional.

NothingEverHappens · 2019-09-30T19:24:31Z

libs/slides/src/lib/deck/deck.component.ts

-    this.goToSlide(this.activeSlideIndex + 1);
+    if (this.activeSlideIndex + 1 < this.slides.length) {
+      this.goToSlide(this.activeSlideIndex + 1);
+    } else if (this.nextLink != null && this.nextLink !== '') {


if(this.nextLink)

noted will change

NothingEverHappens · 2019-09-30T19:25:16Z

libs/slides/src/lib/deck/deck.component.ts

+    return this.activeSlideIndex > 0 || (this.previousLink != null && this.previousLink !== '');
+  }
+
+  public setupPreviousNext(allRoutes: string[]) {


Let's keep the presentation simple:
setNextLink()
and
setNextLink()
All the router manipulations should be done outside of this component, as it is not supposed to know about the codelab routing structure

libs/slides/src/lib/deck/deck.component.ts

libs/slides/src/lib/arrows/slides-arrows.component.ts

kirjs · 2019-10-03T13:13:05Z

apps/codelab/src/app/components/slides/title-slide/title-slide.component.ts

+    if (currentUrl.startsWith('/')) {
+      currentUrl = currentUrl.substr(1);
+    }
+    const urlPaths = currentUrl.split('/');
+    if (urlPaths.length > 1) {
+      const idx = allRoutes.indexOf(urlPaths[1]);
+      if (idx > 0) {
+        previousLink = `/${urlPaths[0]}/${allRoutes[idx - 1]}`;
+      }
+      if (idx < allRoutes.length - 1) {
+        nextLink = `/${urlPaths[0]}/${allRoutes[idx + 1]}`;
+      }


This looks very complicated, is there some activeRoute property we could use to avoid all the string manipulations?

It is complicated for sure, if I can get or compute the full path from menu routes that will work too.

apps/codelab/src/app/components/slides/title-slide/title-slide.component.ts

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

apps/codelab/src/app/components/slides/title-slide/title-slide.component.ts

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.spec.ts

…from title slide. change tests to use spies instead of checking value

NothingEverHappens · 2019-10-04T17:48:13Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+    private readonly presentation: SlidesDeckComponent,
+    @Optional() @Inject(MENU_ROUTES) readonly menuRoutes
+  ) {
+    if (this.menuRoutes != null) {


At which point would menuRoutes be null?

NothingEverHappens · 2019-10-04T17:48:25Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+    }
+  }
+
+  private setupPreviousNext() {


This name seems outdated

NothingEverHappens · 2019-10-04T17:49:34Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+    let currentUrl = this.router.url;
+    if (currentUrl.startsWith('/')) {
+      currentUrl = currentUrl.substr(1);
+    }


This code looks very complicated, what exactly is happening here?

NothingEverHappens · 2019-10-04T17:49:44Z

apps/codelab/src/app/components/slides/title-slide/title-slide.component.spec.ts

+    nextLink: '',
+
+    setPrevious(link) {
+      this.previousLink = link;


What happened to the spies?

NothingEverHappens · 2019-10-04T17:50:32Z

apps/codelab/src/app/components/slides/title-slide/title-slide.component.spec.ts

+    previousLink: '',
+    nextLink: '',
+
+    setPrevious(link) {


just do setPrevious: createSpy(...)

… meaningful values

NothingEverHappens · 2019-10-24T16:03:08Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.spec.ts

  });
+
+  it('should call setNextLink', () => {
+    expect(slidesDeckComponentStub.setNext).toHaveBeenCalled();


Can we verify that it has been called with the proper argument?

NothingEverHappens · 2019-10-24T16:03:23Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+    private readonly presentation: SlidesDeckComponent,
+    @Optional() @Inject(MENU_ROUTES) readonly menuRoutes
+  ) {
+    if (this.menuRoutes != null) {


NothingEverHappens · 2019-10-24T16:04:29Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+      .find(r => r && (r as MenuRoute).prod);
+    const index = this.menuRoutes.findIndex(c => c.path === config.path);
+    let nextLink = '';
+    if (index < this.menuRoutes.length - 1) {


I'm curious what's the value of this check?

NothingEverHappens · 2019-10-24T16:05:29Z

apps/codelab/src/app/components/slides/title-slide/title-slide.component.ts

+  }
+
+  private setupPrevious() {
+    const config = this.activeRoute.snapshot.pathFromRoot


We're we planning to move this logic to a shared service since we're actually just copy-pasting it accross 2 components?

…xt and previous links

…into lesson_1024

NothingEverHappens · 2019-11-25T20:14:17Z

apps/codelab/src/app/codelabs/angular/menu-route.service.ts

+  }
+
+  private getCurrentIndex(activeRoute: ActivatedRoute) {
+    // TODO: inject ActivatedRoute but figure out a way to fix snapshot update issue


Would be awesome for this comment to be more detailed so that next person who sees this knows what the issue is and how to reproduce

apps/codelab/src/app/codelabs/angular/menu-route.service.ts

NothingEverHappens · 2019-11-25T20:16:09Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+    if (this.presentation != null) {
+      let nextLink = this.menuRouteService.getNextLink(this.activeRoute);
+      if (nextLink) {
+        nextLink = '../../' + nextLink;


There should also prob be a TODO here to make it more generic

apps/codelab/src/app/codelabs/angular/menu-route.service.ts

NothingEverHappens · 2019-11-25T20:16:59Z

apps/codelab/src/app/components/slides/title-slide/title-slide.component.ts

+        this.activeRoute
+      );
+      if (previousLink) {
+        previousLink = '../../' + previousLink;


We always do this for both next and previous link, I think this should move in

NothingEverHappens · 2019-11-25T20:17:36Z

libs/slides/src/lib/deck/deck.component.ts

  constructor(
    private readonly cdr: ChangeDetectorRef,
+    private readonly router: Router,
    @Optional() private readonly route: ActivatedRoute


nit: let's call (active|activated)Route to be consistent

apps/codelab/src/app/components/slides/title-slide/title-slide.component.ts

NothingEverHappens · 2019-11-25T20:20:32Z

apps/codelab/src/app/codelabs/angular/menu-route.service.ts

+    if (config == null) {
+      return -1;
+    }
+    const index = this.menuRoutes.findIndex(c => c.path === config.path);


No need for an intermediate var, just return?

NothingEverHappens · 2019-11-25T20:20:53Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+    if (this.presentation != null) {
+      let nextLink = this.menuRouteService.getNextLink(this.activeRoute);
+      if (nextLink) {
+        nextLink = '../../' + nextLink;


There should also prob be a TODO here to make it more generic

NothingEverHappens · 2019-12-16T00:42:24Z

apps/codelab/src/app/codelabs/angular/menu-route.service.ts

+  }
+
+  private getMenuRouteByIndex(index: number) {
+    if (index >= 0 && index < this.menuRoutes.length) {


Do we need these checks? Can it be just
return this.menuRoutes[index]; ?

NothingEverHappens · 2019-12-16T00:43:07Z

apps/codelab/src/app/components/slides/closing-slide/codelab-closing-slide.component.ts

+  ) {
+    if (this.presentation != null) {
+      const nextLink = this.menuRouteService.getNextLink(this.activeRoute);
+      this.presentation.setNext(nextLink);


Drop empty ngOnInit while you're here?

…ondition

sunilj74 added 2 commits September 12, 2019 22:20

enabled arrow icons to jump to previous and next lessons

6b8794c

Merge branch 'master' into lesson_1024

7798eb3

kirjs requested changes Sep 14, 2019

View reviewed changes

sunilj74 added 3 commits September 25, 2019 11:39

Enabled next and previous buttons to navigate to different lessons on…

d3758f2

… the first and last slides

Merge branch 'lesson_1024' of https://github.com/codelab-fun/codelab …

0667163

…into lesson_1024

made presentation slide optional in constructor in title and closing …

e3be024

…slide

NothingEverHappens approved these changes Sep 28, 2019

View reviewed changes

NothingEverHappens approved these changes Sep 30, 2019

View reviewed changes

added tests to title and closing slides

bfc44b7

kirjs requested changes Oct 3, 2019

View reviewed changes

sunilj74 added 3 commits October 3, 2019 10:13

dropped calls to setPrevious from closing slide and calls to setNext …

89831ea

…from title slide. change tests to use spies instead of checking value

merged with master

e84f72b

fixed linting errors

caeb0bc

NothingEverHappens requested changes Oct 4, 2019

View reviewed changes

sunilj74 added 3 commits October 11, 2019 08:36

Changed to createSpy in tests. Also changed some method names to more…

60f6141

… meaningful values

Merge branch 'master' into lesson_1024

5fe9484

removed complicated code from title and closing slides

52958ef

NothingEverHappens requested changes Oct 24, 2019

View reviewed changes

sunilj74 and others added 8 commits November 10, 2019 21:27

moved previous and next link features to a service

feb7744

added commit logs

ef0f7d5

Merge branch 'master' into lesson_1024

e91ae03

Merge branch 'master' into lesson_1024

d9ad958

changed menu-route service to receive ActivatedRoute while getting ne…

f524ec9

…xt and previous links

Merge branch 'master' into lesson_1024

28ddb49

Merge branch 'lesson_1024' of https://github.com/codelab-fun/codelab …

4126829

…into lesson_1024

fixed tests for menu-route.service.ts

65c6403

NothingEverHappens requested changes Nov 25, 2019

View reviewed changes

added more details to TODO action. moved redundant code into functions

f5acd63

Merge branch 'master' into lesson_1024

c9c3956

NothingEverHappens requested changes Dec 16, 2019

View reviewed changes

sunilj74 added 2 commits December 15, 2019 20:39

switched variables to direct returns. also removed an unecessary if c…

5bde57b

…ondition

removed a wrong TODO

154b746

Conversation

sunilj74 commented Sep 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirjs commented Sep 14, 2019

Uh oh!

kirjs commented Sep 14, 2019

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants