[Router]Feedback on documentation

[DeborahK]

As I'm working through the current "Routing & Navigation" chapter, I have the following questions/issues:

1. When discussing the baseRef, the document states: "If the app folder is the application root, ..." What if it is not? And what if it is different from development to production? Is there a way to define this so it can be easily changed as part of the production build process?
2. The documentation states "We must register router providers in bootstrap." It would be helpful for the documentation to explain why.
3. The section on redirecting routes says "We can arrange for that behavior in several ways." Then it proceeds to only show one way. What are some other ways?
4. Missing word(s): "The router supports multiple named outlets, a feature we'll cover in future."
5. The router configuration constant name is camelCase here:
   export const routes: RouterConfig
   But PascalCase here:
   export const HeroesRoutes: RouterConfig
6. The routes variable when updated to merge the heroes routes should also be typed:
   export const routes = [ ...HeroesRoutes, { path: 'crisis-center', component: CrisisListComponent } ];
7. Consider showing the required imports for the code shown in Milestone Switch license to Apache 2 #2 here: "app/heroes/hero-list.component.ts (Constructor)"
8. Consider showing the required imports for the code shown in Milestone Switch license to Apache 2 #2 here: "app/heroes/hero-detail.component.ts (Constructor)"
9. Typo: "The router checks the CanDeactive guards first," should be CanDeactivate.
10. This code mentions that these properties are good to know about, but then does not provide any information on what they are:

    // Not using but worth knowing about
    next:  ActivatedRouteSnapshot,
    state: RouterStateSnapshot
  )

11. Since the DialogService is required to run the application being built in this chapter, consider including it in the tabbed code result at the end of Milestone Change link on the homepage to point to the ng-conf playlist #3.
12. This is more of an implementation question, so may not go here ... but why is getting a route parameter:
    this.route.params.subscribe
    But getting a route query parameter:
    this.router.routerState.queryParams.subscribe
    Seems for discoverability that these should come from a similar root? Consider detailing why this is so in the documentation?

[wardbell]

1. Yes you can change it (that's implied) to whatever is necessary. Might have to after the CLI shakeup. Did you notice the bit about how to set it dynamically ... for plunkers?

2. Helpful perhaps ... but this is a long chapter and the explanation would be lengthy, sad, unconvincing, annoying, confounding, and depressing. Let's leave it as is.

3. Fixed in an update that I should be published soon (as in "tomorrow")

4. What words are missing? You think I should say "in the future"? That's certainly not necessary in English.

5. You're right. You're right about all of the routing constants. If they're going to be constants they should be in UPPER_SNAKE_CASE. We have a mix of lower camelCase and PascalCase. Both are wrong. Let's get that fixed. I think they should be constants so they should be UPPER_SNAKE_CASE.

6. Why? What would that serve? And TS type inference will tell you.

7 & 8) I tried all kinds of ways to show the imports with the ctor ... and I couldn't find anything that didn't obscure the message. The full code with imports is available in the milestone code tabs. I think I'll let this sit until/unless we learn it is a real problem.

9. Thanks. See PR docs(router): changes suggested by DeborahK in #1808 #1837

10. Placeholder for future discussion. But I think you're right to question having them here as a tease. Removing them. See PR docs(router): changes suggested by DeborahK in #1808 #1837

11. Would have included by makeExampleTabs can show a maximum of 8 tabs. We could fix THAT ... but didn't want to go there right now.

Therefore, the user will have to get from plunker if s/he really wants it ... which s/he shouldn't. Because it's NOT the kind of service you'd want to emulate. It's a hacky mock. No real loss by omitting it.

12. It's a good question. The short answer is that the QueryParams belong to the entire route, from top to leaf (and thus come from the RouteState) whereas the params belong to the route segment, known as the ActivatedRoute.

You can see by this answer that it just begs for more discussion about these distinctions. We didn't get into that yet. We will when we cover matrix parameters.

[wardbell]

To your list, I'm adding:

• interfaces.ts should be something else; that term is too broad. See next.
• maybe have a folder for top level routing stuff called /routing and move the loose stuff into it.
• The current contents of interfaces.ts are really guard service classes and interfaces.Maybe rename this one can-deactivate-guard.service maybe.
• .guard isn't an "approved" extension in the StyleGuide. Rename classes with approved .service extension.
• The .routes extension has been approved and will be added to the StyleGuide
• We should follow the StyleGuide and change all constants to UPPER_SNAKE_CASE. That means HEROES_ROUTES (or HERO_ROUTES?), not HeroesRoutes. And export const ROUTES, not routes.

No PR for these yet.

[Foxandxss]

2. those kind of things are the perfect match for the style guide.

[DeborahK]

Thanks for looking through these. Regarding your reply to my list:

1. It would be great to see an example.
2. In all other places it has wording such as "in a future update".
3. Because it is in every other place in this document ... just not this one. (Search for "export const")

[wardbell]

1. See how we do it for plunker, as described here

2. meh :-)

3. Fixed in PR

4. PR docs(style-guide): spell const variables in lower camel case #1848 changes style recommendation for const variable naming to lowerCamelCase

[wardbell]

Closing after merging #1848.

If you have new recommendations, please create a new issue.