This repository has been archived by the owner. It is now read-only.

Add "set title" cookbook by Ben Nadel #1069

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@wardbell
Contributor

wardbell commented Apr 10, 2016

DO NOT MERGE. Work in progress.

  • Ben to OK it
  • Add e2e test

@googlebot googlebot added the CLA: yes label Apr 10, 2016

@wardbell

This comment has been minimized.

Contributor

wardbell commented Apr 10, 2016

Hi Ben -
Great first-time cookbook!

I've tweaked it to get around the rough spots, illustrate some of our elements of style, and open some possibilities for the future.

I linked Title back to the API guide. You can see how for future reference.

The plunker is working now. I'm not sure what was wrong or how I fixed it. There were some HTML tags that weren't quite correct (<link> doesn't take a closing tag although most browsers ignore that).

It may have been the tabs. All of our code samples use two spaces for tabs. I converted your code accordingly. Please set your environment to do that automagically.

Other style points

  • We use single quotes for strings in our code.

  • We use less vertical white space and fewer comments than you prefer. I like your approach, especially for your blog. But our chapters are already so huge that we trim that. Another advantage of TypeScript is that the reader can expect more feedback in a supportive IDE so we don't have to tell them how the code works so much.

  • We always use "we" instead of "you" except in an aside where we're allowing them to go their own way as in "You could do 'x' but we'd rather not".

  • We don't reference Angular 1 except in extraordinary situations (and the A1 -> A2 cookbook of course!). We think others outside the docs should and will do that. But our docs try to stand alone and really be about Angular 2 (which we always refer to simply as Angular).

  • The alert and callout don't work that well, especially for long text. We tend to pull them into the .l-sub-section area or, as I did in your point about the different Title services, pull it into a section of its own, perhaps as an appendix at the bottom.

  • I carve up the samples with more #docregion tags to focus the discussion more. I often use the +makeTabs plugin to provide the full source code at the bottom when that feels appropriate. I show how in my tweaks.

  • The compiler complained when you set _titleService in the ctor without first declaring it. We often leverage TypeScript's ability to declare and set a variable in the ctor params like this:

    public constructor(private _titleService: Title ) { }
    
  • I love your videos! We aren't that bold. But we often illustrate running code with animated gifs. I made one for you.

  • We list cookbook topics alphabetically. We lack automation for that. I moved this entry in the _data.json

  • An admin step that I do that you don't have to worry about is make sure there are entries for this cookbook in the JS and Dart language tracks (see their _data.json files and the placeholder jade.

Just me

As other author's know, I have a hard time keeping my hands off the prose. I elaborated on why you are right to register the Title service in bootstrap when we almost always register application wide services in the root App.Component.

I added the observation that one could grab the document object and wrestle with the DOM directly ... and why we discourage that.

The plunker doesn't show the title changing. This is a problem I first encountered with routing. I provided a little instruction to show you can see the change in pop-up window mode.

Tell me when it's ready!

There is a future task that I hid in Jade comments ... how to use the Title service to set the back/forward buttons in navigation. Maybe you'll take that on at some point. We can talk about it.

But I'd be happy to publish now ... or as soon as you give the signal that you think it is ready ... and someone adds an e2e test

Cheers,
W

@bennadel

This comment has been minimized.

Contributor

bennadel commented Apr 11, 2016

@wardbell thank you for the in-depth feedback. I can totally get on board with the syntax and prose style for the docs. And certainly no worries about editing the prose yourself - I assume you have a better holistic sense of continuity than anyone else here, so do what you think is right.

I did try looking at the e2e testing, but was very unclear on how they worked (I'm honestly not a big tester yet). I will try to dig in a bit more.

@bennadel

This comment has been minimized.

Contributor

bennadel commented Apr 11, 2016

Also, is there a tool you use to create those GIFs? And, is the addition to the /resources automated somehow? Or are you just committing a file there manually?

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Apr 11, 2016

You can use snagit or if you use mac, gifgrabber. Then simply move the .gif into resources and commit it.

@bennadel

This comment has been minimized.

Contributor

bennadel commented Apr 11, 2016

@Foxandxss awesome, thanks!

@bennadel

This comment has been minimized.

Contributor

bennadel commented Apr 13, 2016

@wardbell I think the edits you made are solid. I left one question about one of the code-snippets; but otherwise, I think I understand the style that you guys are using.

I still have to add the e2e tests. I will get that done this week - but may need some help with figuring out how to do that.

After this is all done, I will be much better with my iterations - once I have my bearings, it will flow more naturally.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Apr 13, 2016

@bennadel If you have too much trouble with the e2e, just ping me and I help you.

@bennadel

This comment has been minimized.

Contributor

bennadel commented Apr 13, 2016

Awesome, thanks!

@wardbell

This comment has been minimized.

Contributor

wardbell commented Apr 14, 2016

Where did I do <title>{{ pageTitle }}</title>???

I thought I was pretty darned clear that wouldn't work ... although I neglected to make it look like a binding so I MUST do that to complete the example.

<title>{{This_Does_Not_Work}}</title>

Do you see any ambiguity there? ;-)

bootstrap(AppComponent, [ Title ])
// #enddocregion bootstrap-title
.then(
() => window.console.info( 'Angular finished bootstrapping your application!' ),

This comment has been minimized.

@ocombe

ocombe Apr 18, 2016

Collaborator

you should remove the reference to window here to make sure it works on all environments, and you already use console directly in all the other files

This comment has been minimized.

@bennadel

bennadel Apr 18, 2016

Contributor

@ocombe In general, I would agree. But, in this case, I'm using the Browser bootstrap. If I were to use a different bootstrap, I think that's when you would need to change the way the promise is handled?

This comment has been minimized.

@ocombe

ocombe Apr 18, 2016

Collaborator

Oh true :)

@bennadel

This comment has been minimized.

Contributor

bennadel commented Apr 19, 2016

@wardbell @Foxandxss I added an e2e-test. Would one of you be so kind as to take a peek and let me know if this is what is expected?

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented Apr 19, 2016

@bennadel It is lovely.

@wardbell

This comment has been minimized.

Contributor

wardbell commented Apr 23, 2016

Sweet! Merging it now

@wardbell wardbell changed the title from [WIP] Add "set title" cookbook by Ben Nadel to Add "set title" cookbook by Ben Nadel Apr 23, 2016

@wardbell wardbell closed this in c9d4050 Apr 23, 2016

@wardbell wardbell deleted the IdeaBlade:bennadel-set-title branch Apr 23, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.