Skip to main content
Code Review

Return to Answer

added 215 characters in body
Source Link
H3AR7B3A7
  • 239
  • 2
  • 12

Overall your code already has a decent structure to it. Here are some pointersof the more obvious things that I think could be improved, after taking a look at your project. Some are rather small things that you could easily fix, others are things you might want to adopt over time.

this.router.navigate(['..']); // <--- You can also use the route as defined in your app-routing.module.ts

The styling of your app is rather simple but pretty, apart from some buttons hugging each other. However, all your styling is in the global styles.scss file. Angular uses encapsulation and you should want to make use of it. Style the presentation components in the stylesheet at the component level. Only actual global styles should go in the styles.scss file. Namely your font, something to normalize your styling and other base styles (like for example your company/house/angular-material default style imports).

Here are some pointers after taking a look at your project.

this.router.navigate(['..']);

The styling of your app is rather simple but pretty, apart from buttons hugging each other. However, all your styling is in the global styles.scss file. Angular uses encapsulation and you should want to make use of it. Style the presentation components in the stylesheet at the component level. Only actual global styles should go in the styles.scss file. Namely your font, something to normalize your styling and other base styles (like for example your company/house/angular-material default style imports).

Overall your code already has a decent structure to it. Here are some of the more obvious things that I think could be improved, after taking a look at your project. Some are rather small things that you could easily fix, others are things you might want to adopt over time.

this.router.navigate(['..']); // <--- You can also use the route as defined in your app-routing.module.ts

The styling of your app is rather simple but pretty, apart from some buttons hugging each other. However, all your styling is in the global styles.scss file. Angular uses encapsulation and you should want to make use of it. Style the presentation components in the stylesheet at the component level. Only actual global styles should go in the styles.scss file. Namely your font, something to normalize your styling and other base styles (like for example your company/house/angular-material default style imports).

Source Link
H3AR7B3A7
  • 239
  • 2
  • 12

Here are some pointers after taking a look at your project.

Use Angular Router

You shouldn't navigate by url within the angular application.

Change:

this.router.navigateByUrl('/');

To:

this.router.navigate(['..']);

And you will notice way faster routing using the angular router. Your approach will reload the whole app, which is defeating the purpose of a SPA.

Architecture

Your contact manager component handles both the data and presentation in a list. Try working in a container-presentation components approach. Once you have multiple features, you might also want them to have their own module so you can lazy load them.

Container Component

  • Manages the state by interacting with service/store
  • Contains only minor presentation, like for example a header/title
  • Passes state to presentation components using input properties

Presentation Component

  • Presents/renders the data, but does not directly interact with state
  • Communicates with container component using output properties
  • Use child routes to load them into a router-outlet in your container component

After that you might want to consider using the On-Push change detection strategy. Using the inputs for pushing the data to your presentation components will help a lot with getting ready for this. The input trigger the OnPush change detection.

Unsubscribing

Instead of subscribing to the data you get from your back end you could use the async pipe. Right now you are subscribing to your observables, but you never unsubscribe. You could do this in a couple of ways:

  • Unsubscribe in ngOnDestroy
ngOnInit() {
 this.sub = this.someEventBus.on(
 Events.SomeEvent, (something => this.something = something))
}
ngOnDestroy() {
 if (this.sub) {
 this.sub.unsubscribe()
 }
}
  • AutoUnsubscribe decorator
@AutoUnsubscribe()
export class SomeComponent implements OnDestroy {
 sub: Subscription
 constructor(private someEventBus: SomeEventBusService) { }
 ngOnInit() {
 this.sub = this.someEventBus.on(
 Events.SomeEvent, (something => this.something = something))
 }
}
  • SubSink ( yarn add subsink)
subs = new SubSink()
constructor(private someEventBus: SomeEventBusService) { }
ngOnInit() {
 this.subs.sink = this.someEventBus.on(
 Events.SomeEvent, (something => this.something = something))
}
ngOnDestroy() {
 this.subs.unsubscribe()
}
  • Use the takeUntil operator to complete observables instead of unsubscribing.
private stopFiring = new Subject()
someObservable$.pipe(
 // Do all the things...
 takeUntil(this.stopFiring)
);
// To complete the observable
this.stopFiring.next()
  • Let async pipes handle subscribing / unsubscribing

Async pipe

I really recommend using these and let Angular handle the subscribing/unsubscribing. It takes a little getting used to, but I tend avoid manually subscribing altogether most of the time using this approach.

For example:

Change this:

public getContactsPaginated() {
 this.contactService
 .getContactsPaginated(this.currentPage, this.itemsPerPage)
 .subscribe({
 next: (response) => {
 this.numberOfContactsInDB = +response.headers.get('X-Total-Count')!;
 this.contacts = response.body!;
 this.loadedContacts = response.body!;
 this.contactsFiltered = response.body!;
 this.calculatePages();
 this.changePageButton();
 },
 });
 }

To something like this:

contactsPaginated$ = this.contactService
 .getContactsPaginated(this.currentPage, this.itemsPerPage)
 .pipe(
 tap(response => this.numberOfContactsInDB = +response.headers.get('X-Total-Count')!;)
 )

And in your template:

*ngFor="let contact of (contactsPaginated$ | async).contactsFiltered"

Or use some more rxjs operators to mold the response in the object you want first:

contactsPaginated$ = this.contactService
 .getContactsPaginated(this.currentPage, this.itemsPerPage)
 .pipe(
 tap(response => this.numberOfContactsInDB = +response.headers.get('X-Total-Count')!),
 .pluck('contactsFiltered'),
 .map(obj => /* Your mapping here... */)
 )

This also takes some getting used to, but the more you use it the more you will start to appreciate what you can do with RxJs without cluttering all over your component.

File naming

Naming convention for files holding your models looks like this: contact.model.ts

Styling

The styling of your app is rather simple but pretty, apart from buttons hugging each other. However, all your styling is in the global styles.scss file. Angular uses encapsulation and you should want to make use of it. Style the presentation components in the stylesheet at the component level. Only actual global styles should go in the styles.scss file. Namely your font, something to normalize your styling and other base styles (like for example your company/house/angular-material default style imports).

lang-js

AltStyle によって変換されたページ (->オリジナル) /