-
Notifications
You must be signed in to change notification settings - Fork 10
Exercise 4 - Templates - Socks Shop #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Exercise 4 - Templates - Socks Shop #16
Conversation
@for(sock of socks$ | async; track sock.id){ | ||
<div class="col-sm-6 col-md-4 col-lg-3"> | ||
<div class="box"> | ||
<a href="/socks/{{ sock.id }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hier wel opletten!!
Als je een <a href="">
doet, dan gaat de browser daar naartoe navigeren, je hebt daarmee een page reload!!
Ofte: je verliest de app state + een re-render van de volledige pagina
Je moet met routerLink
werken zodat je binnen de SPA blijft!
@@ -24,4 +41,8 @@ <h6><span>{{ sock.price }}</span></h6> | |||
</div> | |||
</div> | |||
</div> | |||
<div> | |||
<button (click)="onPageChange(currentPageSubject.value - 1)" [disabled]="currentPageSubject.value === 1">Previous</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use van [disabled]
!!
public colorFilterSubject = new BehaviorSubject<string>(''); | ||
public sortBySubject = new BehaviorSubject<string>('name'); | ||
public currentPageSubject = new BehaviorSubject<number>(1); | ||
public pageSize = 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public is default, je hoeft dit dus niet speciaal te vermelden
Op sommige projecten doen ze dat wel --> volg dan de team/project standaarden
} | ||
|
||
onFilterColor(event: Event) { | ||
const color = (event.target as HTMLInputElement).value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dit is een beetje een probleem in Angular, je zou hierrond kunnen werken met wat TypeScript magic:
public onChange(event: Event & { target: HTMLInputElement }): void { }
Maar of dat dan beter is....
const startIndex = (currentPage - 1) * this.pageSize; | ||
return filtered.slice(startIndex, startIndex + this.pageSize); | ||
}) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Components, components, components!!
Je zit in een vrij groot ShopComponent
waarin nu vanalle logica komt te staan (paginatie, filtering, sorting, ...)
En later ook nog 101 andere dingen
Zulke components zijn moeilijk testbaar (als er ooit testen zouden geschreven worden ;) )
En ook moeilijk begrijpbaar, er is wat vanalles aan de gang. Na een tijdje heb je geen idee meer voor welke functionaliteit welke code precies verantwoordelijk is
- in sommige functies (vb ngOnInit) zit er voor elk van die dingen wat code...
SRP --> Maak meer en kleinere componenten die elk een heel duidelijk afgelijnd doel/purpose hebben
No description provided.