Skip to content

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jonathanrasquin
Copy link

No description provided.

@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 }}">
Copy link
Member

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>
Copy link
Member

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;
Copy link
Member

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;
Copy link
Member

@Laoujin Laoujin May 31, 2024

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);
})
);
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants