Skip to content

Pavel mihailov #5

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

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Pavel mihailov #5

wants to merge 18 commits into from

Conversation

pavelsm-ctco
Copy link

No description provided.

loadChildren: () =>
import('./posts/posts.routes').then((mod) => mod.routes),
providers: [
PostsService,
Copy link

@up-ctc up-ctc May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Services that have @Injectable({ providedIn: 'root'}) should be not provided separately. Providing in root means service Available on whole app level and is alive while app is alive. This will also guarantee that data is persisted when you are switching between routes.

In current approach there are actually 2 PostsService. One on App level and then this route component has its own.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx. removed.

<mat-label>Message</mat-label>
<textarea matInput formControlName="message" rows="3">
</textarea>
@if (message.hasError("minlength") || message.hasError("maxlength") || message.hasError("required")) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to change it now, just suggestion for future ->
It's always better to extract more complex logic to separate component function because it is easier to unit test it (rather than test this behavior in component), but it is also correct. Just hint from my work experience. :)

}

ngOnInit(): void {
this.doSubmitSubscription = this.doSubmit$.subscribe(this.onSubmit.bind(this));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI another good solution how to take care of unsubscribes (its useful if you have several subscriptions).

`private subscriptions= new Subscription();

this.subscriptions.add (
this.doSubmit$.subscribe(...
);

ngOnDestroy() {
this.subscriptions.unsubscribe()}

Then you will be able to handle several subscriptions at once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx, i've changed to the suggested approach. In our project i have not see this approach. It is a lot more similar .NET winforms events.


@Component({
selector: 'app-posts-list',
imports: [NgIf, MatCardModule, RouterLink, MatButtonModule],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use new @if / @for syntax instead of old approach directives then no need to import ngIf or Common module:
https://angular.dev/essentials/templates#control-flow-with-if-and-for


private breakpointObserver = inject(BreakpointObserver);

isHandset$: Observable<boolean> = this.breakpointObserver.observe(Breakpoints.Handset)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it used somewhere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

) {}

ngOnInit(): void {
if (this.el) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.el should always be truthy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified IF statement

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