Separate operators in multiple pages #161
Conversation
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
==========================================
- Coverage 91.02% 85.71% -5.32%
==========================================
Files 7 8 +1
Lines 78 91 +13
Branches 5 4 -1
==========================================
+ Hits 71 78 +7
- Misses 6 13 +7
+ Partials 1 0 -1
Continue to review full report at Codecov.
|
| .fragment | ||
| .subscribe(name => this.scrollToOperator(name)); | ||
| this._subscription = this._activatedRoute.fragment.subscribe(name => | ||
| this.scrollToOperator(name) |
There was a problem hiding this comment.
@feloy - This subscription is not required anymore since we don't use fragments.
src/app/services/seo.service.ts
Outdated
| constructor(private _title: Title, private _meta: Meta) {} | ||
|
|
||
| public setHeaders(titleParts: string[], description: string) { | ||
| this._title.setTitle([...titleParts, this.siteTitle].join(' \u2022 ')); |
There was a problem hiding this comment.
Right now the title is combine all combination RxJS Documentation . I would have expected the reverse, just wanted to confirm.
There was a problem hiding this comment.
Yes, it is recommended to put most specific words at the beginning of the title
src/app/services/seo.service.ts
Outdated
| public homeDescription = 'The complete RxJS documentation...'; | ||
| public operatorsDescription = 'All the RxJS operators...'; | ||
| public companiesDescription = 'Companies that use RxJS...'; | ||
| public teamDescription = 'People behind the RxJS Documentation project...'; |
There was a problem hiding this comment.
Would it be better to utilize an const Object for page seo meta data, something like
{
home : {
title : 'Home page title',
description: 'Home page description',
},
.....
}
This way we can access it per page
There was a problem hiding this comment.
Why not store this data in the route config and listen to router events to update the title and SEO instead of setting it from each component?
There was a problem hiding this comment.
I agree, this seems ideal. ☝️
There was a problem hiding this comment.
Do you agree that I make it the way it is done in this sample app, defining the data in the route config directly (via an external file if we want to facilitate translations) and processed with guards:
https://github.com/feloy/ng-universal-demystified/blob/master/src/app/app.module.ts
There was a problem hiding this comment.
I agree with using the route config but don't think it needs to be processed with guards. You can get the activated route from the route tree in the NavigationEnd event.
Here is an plunker: http://plnkr.co/edit/6iVjZcnqKBi7Dpp3KrFX?p=info
The components.ts has the example code.
|
@feloy - This is awesome ! just few comments and questions. |
|
Thanks @ashwin-sureshkumar I'll work on the comments during the week. |
|
😍👯 |
| private _router: Router, | ||
| private _activatedRoute: ActivatedRoute, | ||
| @Inject(OPERATOR_TOKEN) public operators: OperatorDoc[], | ||
| private _seo: SeoService |
There was a problem hiding this comment.
Since this is an Angular project I believe it's worth sticking to their official style guide that discourages from prefixing private variables with _. See https://angular.io/guide/styleguide#style-03-04
There was a problem hiding this comment.
This is debatable, in the material docs project _ is being used for private. I prefer this convention.
There was a problem hiding this comment.
I prefer to stick to the existing code in the project and use _
| import { Router, ActivatedRoute } from '@angular/router'; | ||
| import { SeoService } from '../../../services/seo.service'; | ||
| import { OperatorDoc } from '../../../../operator-docs/operator.model'; | ||
| import 'rxjs/add/operator/pluck'; |
There was a problem hiding this comment.
Maybe it'll be better to use only lettable operators when this project is already using RxJS 5.5.2?
There was a problem hiding this comment.
hi @martinsik and thanks for popping in to say hi! :) thx for reviewing this PR.
| this.operator = | ||
| this.operators.filter( | ||
| (operator: OperatorDoc) => operator.name === name | ||
| )[0] || this.notfound(); |
There was a problem hiding this comment.
Can we use a map here rather than filtering through array each time?
|
I would like to add some tests, otherwise I think I've replied to all your comments. |
| this.operators.filter( | ||
| (operator: OperatorDoc) => operator.name === name | ||
| )[0] || this.notfound(); | ||
| if (name in this.operatorsMap) { |
There was a problem hiding this comment.
@feloy - Can use Map instead of object to create a map of operators. This works as well, but map being explicit. Just a suggestion
|
@sumitarora @btroncone - Can you please review when you get a chance ? |
|
@feloy Can you rebase it and have 1 commit for all the work. |
|
@ashwin-sureshkumar Reviewed and approved just if @feloy Can rebase it that would be great else should be good to merge. |
|
Ok, I'll make a rebase asap |
|
so lovely! this is great @feloy! |
8 participants
Refs issue #150
VSCode has made some reformatting (mostly " -> '), I can revert them if needed.