feat(angular-ngrx-scss): Create main layout (Top repos + Gists) by SDaian · Pull Request #319 · thisdot/starter.dev-github-showcases

SDaian · 2022-05-27T18:49:24Z

Description

Get Gists from GitHub API
Create main page layout (Top repositories + gists)

Fixes: #117
Fixes: #106
Fixes: #108

Screenshots

aws-amplify-us-east-1 · 2022-05-27T18:51:49Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-319.dbm2zhgk5abrj.amplifyapp.com

aws-amplify-us-east-1 · 2022-05-27T18:51:57Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-319.d1190gmno2hljg.amplifyapp.com

aws-amplify-us-east-1 · 2022-05-27T18:52:28Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-319.d3jint31f72euh.amplifyapp.com

aws-amplify-us-east-1 · 2022-05-27T18:53:01Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-319.d3sqkonra6x7y9.amplifyapp.com

Amdrel

Per Dustin's response in Slack we should aim for at least 80% code coverage (ideally 100%) in PRs. Could you write some tests for this new functionality?

lindakatcodes

Left a few questions and requests for you. :) Let me know if there's anything that's not clear!

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.html

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.scss

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.ts

angular-ngrx-scss/src/app/user/services/user.service.ts

angular-ngrx-scss/src/app/state/user/user.state.ts

angular-ngrx-scss/src/app/state/user/user.selectors.ts

angular-ngrx-scss/src/app/state/user/user.reducer.ts

angular-ngrx-scss/src/app/state/user/user.effects.ts

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.html

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.scss

angular-ngrx-scss/src/app/home/user-gists/user-gists.component.html

angular-ngrx-scss/src/app/state/profile/profile.state.ts

angular-ngrx-scss/src/app/state/user/user.effects.spec.ts

angular-ngrx-scss/src/app/state/user/user.state.ts

Amdrel · 2022-07-01T17:04:44Z

angular-ngrx-scss/src/app/user/services/user.service.spec.ts

+        },
+      },
+    ];
+    httpClientSpy.get.and.returnValue(of(expectedHttpResponse).pipe(delay(0)));


question: Why is this .pipe(delay(0)) needed?

Removed, since now the observables are handled from the fakeAsync approach.

Amdrel · 2022-07-01T17:06:13Z

angular-ngrx-scss/src/app/user/services/user.service.spec.ts

+
+    userService.getUserGists(username).subscribe((res) => {
+      expect(res).toEqual(expectedResponse);
+      done();


nitpick: We should call done() inside of the complete callback of the subscription instead of the success one. If an error happens in the observable, this test may hang and never complete.

I haven't tested this though. Maybe subscriptions work differently in tests and it will bail out intelligently when an error throws.

Completely agree with you @Reshurum, doing a little bit of research found this quite interesting article who mention we should avoid the done() method on the spec. because can cause some false positives and a slow performance on tests:

https://www.piotrl.net/angular-testing-avoid-done/

So basically I rewrite using fakeAsync to handle the Observables.

angular-ngrx-scss/src/app/user/services/user.service.ts

SDaian self-assigned this May 27, 2022

SDaian added this to the Angular + NgRx + SCSS milestone May 27, 2022

SDaian requested review from Amdrel and lindakatcodes June 17, 2022 19:12

SDaian changed the title ~~feat(angular-ngrx-scss): Get gists from Github API~~ feat(angular-ngrx-scss): Create main layout (Top repos + Gists) Jun 17, 2022

SDaian force-pushed the fix/issue-117 branch from cd7c346 to 90574d7 Compare June 17, 2022 19:13

Amdrel requested changes Jun 17, 2022

View reviewed changes

SDaian force-pushed the fix/issue-117 branch from 90574d7 to 8eb0fb2 Compare June 22, 2022 22:56

lindakatcodes suggested changes Jun 22, 2022

View reviewed changes

SDaian force-pushed the fix/issue-117 branch 3 times, most recently from c227cdc to 8138748 Compare June 23, 2022 02:44

SDaian requested review from Amdrel and lindakatcodes June 23, 2022 02:49

ihardz reviewed Jun 27, 2022

View reviewed changes

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.html Outdated Show resolved Hide resolved

angular-ngrx-scss/src/app/home/top-repositories/top-repositories.component.html Outdated Show resolved Hide resolved

lindakatcodes mentioned this pull request Jun 30, 2022

[Angular - NgRx - SCSS] Create gists panel #186

Closed

3 tasks

lindakatcodes force-pushed the fix/issue-117 branch from 8138748 to 804e475 Compare July 1, 2022 15:01

Amdrel requested changes Jul 1, 2022

View reviewed changes

lindakatcodes mentioned this pull request Jul 1, 2022

[Angular - NgRx - SCSS] Refactor of Services, Prep for state restructuring #440

Closed

6 tasks

SDaian force-pushed the fix/issue-117 branch from cbf5fb8 to 484ef4a Compare July 4, 2022 10:17

SDaian added 2 commits July 4, 2022 12:18

feat(angular-ngrx-scss): Get gists from Github API

285d37d

[fix/issue/106] - Add main page layout

27c7bb6

SDaian force-pushed the fix/issue-117 branch from 484ef4a to 2cd429b Compare July 4, 2022 10:18

SDaian requested review from Amdrel and ihardz July 4, 2022 10:19

SDaian force-pushed the fix/issue-117 branch from 2cd429b to b508d75 Compare July 4, 2022 10:30

[fix/issue-106] - Create main page layout

2627196

SDaian force-pushed the fix/issue-117 branch from b508d75 to 2627196 Compare July 5, 2022 15:34

SDaian added 5 commits July 6, 2022 00:29

Add missing changes

77234b3

Remove unnecesary params

6a5a3e5

Update params to the topRepos call

bf00576

Added disabled feature for view-all

d16ebb6

fix format

608fa15

lindakatcodes approved these changes Jul 6, 2022

View reviewed changes

SDaian merged commit a637c22 into main Jul 7, 2022

SDaian deleted the fix/issue-117 branch July 7, 2022 08:43

Conversation

SDaian commented May 27, 2022 • edited by dustinsgoodman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Uh oh!

aws-amplify-us-east-1 bot commented May 27, 2022

Uh oh!

aws-amplify-us-east-1 bot commented May 27, 2022

Uh oh!

aws-amplify-us-east-1 bot commented May 27, 2022

Uh oh!

aws-amplify-us-east-1 bot commented May 27, 2022

Uh oh!

Amdrel left a comment

Choose a reason for hiding this comment

Uh oh!

lindakatcodes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amdrel Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

SDaian Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

Amdrel Jul 1, 2022

Choose a reason for hiding this comment

Uh oh!

Amdrel Jul 1, 2022 • 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!

SDaian Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SDaian commented May 27, 2022 •

edited by dustinsgoodman

Loading

Amdrel Jul 1, 2022 •

edited

Loading