Frontend Developer Test - #11
claudiugabriel15 wants to merge 1 commit into
Conversation
johnfedoruk
left a comment
johnfedoruk
left a comment
There was a problem hiding this comment.
Automated Testing:
- Lint: passes
- test: passes
- coverage: 89/100
- build: pass
Manual Review
- Has Angular animations
- Good use of services
- Has auth guards
- Decent README
- Good use of forms
- Lazy loading modules (
- RxJS looks good
- Use of local storage for a JWT is a security vulnerability - should use sessionStorage
- Decent directory structure
- Good use of typescript features (privacy modifiers / return types / type setting)
| - The Secure Module (direcotry path: src/app/secure) | ||
|
|
||
| Please commit your code regularly. There is no time limit, though we will be looking at the commit metrics. We will be looking for DRY code and good coding practices with high code coverage. Update the README to explain the code-base. | ||
| They are independent of each other and don't share any code. |
| ) {} | ||
|
|
||
| canActivate() { | ||
| return this.authService.checkTokenValidity().then( |
| import { Account } from './models/account'; | ||
| import { HttpClient, HttpHeaders } from '@angular/common/http'; | ||
|
|
||
| import 'rxjs/add/operator/map'; |
There was a problem hiding this comment.
Ps: now RxJS says to use Observable pipes for operators
|
|
||
| @Injectable() | ||
| export class AuthService { | ||
| private loginURL = 'https://dev-test-service.madebywiser.com/login'; |
There was a problem hiding this comment.
This should be loaded from environment - read about defining a static forRoot method in the module class in order to gain access to configuration by the importing module.
| HttpStatusModule | ||
| HttpStatusModule, | ||
|
|
||
| AuthModule, |
There was a problem hiding this comment.
Hint: Importing the AuthModule kills the benefit of lazy loading. AuthModule should be split into two modules - one that contains the views which can be lazy loaded, and one that contains the services/guards.
|
|
||
| const AUTH_ROUTES = [ | ||
| { | ||
| path: 'auth', |
| trigger('loginFailed', [ | ||
| state('true', style({ | ||
| })), | ||
| transition('* => true', [ |
| export class ContentComponent { | ||
| public selectedUserId; | ||
|
|
||
| users = [ |
There was a problem hiding this comment.
Would've been good to make a dumb service to provide this data
| routeSubscription: any; | ||
|
|
||
| // mock user details | ||
| users = [ |
There was a problem hiding this comment.
Would've been good to use a dumb service to provide this
|
|
||
| const SECURE_ROUTES = [ | ||
| { | ||
| path: 'secure', |
There was a problem hiding this comment.
Should be path: '', - otherwise would not work with lazy-loading without first importing into the parent module
2 participants
Completed the assignment.