-
Notifications
You must be signed in to change notification settings - Fork 38
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
Feat/faq #63
base: master
Are you sure you want to change the base?
Conversation
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.
Good work! :-)
We should change service to avoid perform more requests than needed
@@ -6,9 +6,10 @@ | |||
alt="Angular communities logo" | |||
src="assets/images/angular-communities-logo_md.png" | |||
/> | |||
<span>ANGULAR COMMUNITIES</span> | |||
<a routerLink="">ANGULAR COMMUNITIES</a> |
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.
👏👏
src/app/pages/main/main.component.ts
Outdated
communities: Communities; | ||
/* communities: Communities; */ | ||
communitie$ = this.communityService.communities; | ||
community$ = this.route.fragment.pipe(map(community => this.communitie$[community])); |
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 isn't going to work communitie$
is an observable now. I think it should work
community$ = this.route.fragment.pipe(map(community => this.communitie$[community])); | |
community$ = this.route.fragment.pipe(withLatestFrom(this.communitie$), map(([community, communities]) => communities[community])); |
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.
Oops. Fixed in 65e0b60
src/app/pages/main/main.component.ts
Outdated
@@ -10,11 +11,17 @@ import { map } from 'rxjs/operators'; | |||
}) | |||
export class MainComponent { | |||
@Input() | |||
communities: Communities; | |||
/* communities: Communities; */ | |||
communitie$ = this.communityService.communities; |
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 observable is going to perform two requests and more if we move to the new faq
page and come back. We should avoid it using shareReplay(1)
, but it can't be placed here. We should change service, and move communities
getter to simple property (communitie$
?). Something like
communitie$ = this.httpClient
.get<Communities>(this.JSON_COMMUNITIES)
.pipe(map(communities => this.normalizeCommunities(communities)), shareReplay(1));
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.
You are completely right, fixed in f4afebb
src/app/pages/faq/faq.module.ts
Outdated
|
||
@NgModule({ | ||
declarations: [...COMPONENTS], | ||
imports: [CommonModule, SharedModule, FlexLayoutModule, ReactiveFormsModule, MatInputModule], |
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.
All the modules are imported via shared, can be removed 👍
imports: [CommonModule, SharedModule, FlexLayoutModule, ReactiveFormsModule, MatInputModule], | |
imports: [SharedModule], |
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.
Yeah, those modules shouldn't even be imported in faqModule anyway. My mistake, for copy-pasting xD
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.
Fixed in 32a3833
src/app/pages/faq/faq.module.ts
Outdated
const COMPONENTS = [FaqComponent]; | ||
|
||
@NgModule({ | ||
declarations: [...COMPONENTS], |
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.
By the way, it isn't needed spread it, like in SharedModule
declarations: [...COMPONENTS], | |
declarations: [COMPONENTS], |
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.
Once again, my mistake for copy-pasting 😅
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.
Also fixed in 32a3833
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.
LGTM! 👍
font-size: 16px; | ||
} | ||
|
||
.question { |
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.
We should make the design responsive
src/app/app-routing.module.ts
Outdated
|
||
const routes: Routes = [ | ||
{ path: '', component: MainComponent }, | ||
{ path: 'faq', component: FaqComponent }, |
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.
The FAQ module can be built in a lazy way
SonarCloud Quality Gate failed. |
Regarding #59