-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: main
Are you sure you want to change the base?
Conversation
* Add angular material design * Additing navigation component * Removed App Nav component -> App component. Added empty posts and guest book components * Added ngrx. Added Actions, effects, reducer, selectors for Posts.
…ervice. Moved posts routes to a separate file
… for localStorage
src/app/app-routes.ts
Outdated
loadChildren: () => | ||
import('./posts/posts.routes').then((mod) => mod.routes), | ||
providers: [ | ||
PostsService, |
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.
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.
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.
thx. removed.
<mat-label>Message</mat-label> | ||
<textarea matInput formControlName="message" rows="3"> | ||
</textarea> | ||
@if (message.hasError("minlength") || message.hasError("maxlength") || message.hasError("required")) { |
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.
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)); |
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.
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.
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.
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], |
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.
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) |
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.
is it used somewhere?
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.
removed
) {} | ||
|
||
ngOnInit(): void { | ||
if (this.el) { |
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.
this.el should always be truthy.
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.
simplified IF statement
No description provided.