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

docs(cb-barrels): Add barrels cookbook recipe #1419

Closed
wants to merge 42 commits into
base: master
from

Conversation

Projects
None yet
@jmcooper

jmcooper commented May 17, 2016

Add the cookbook recipe demonstrating how to create barrels to simplify module imports.

@googlebot googlebot added the CLA: yes label May 17, 2016

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 17, 2016

I like it, but I would make it follow the style-guide. That means having /heroes/heroes.component.ts|html|css, /heroes/hero-detail/hero-detail.component.ts|html|css. Service renamed as hero.service.ts, model renamed to hero.model.ts and both put into a shared folder. That way you will have more barrels too so the cookbook would be more interesting.

Leaving that aside, I have no problem with the prose :)

EDIT: Service is having the right name, you just mistyped it.
EDIT 2: You can use a .filetree to represent... you file tree.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 17, 2016

Oh, final call here is from @wardbell :)

@jim-cooper

This comment has been minimized.

jim-cooper commented May 17, 2016

I like your suggestions @Foxandxss. I'll make those changes.

@jim-cooper

This comment has been minimized.

jim-cooper commented May 17, 2016

There is one problem I just realized. Adding the SystemJs package in the index.html works great for running the example, locally, but it doesn't work on plunker. Unfortunately, the main needs to be index.js locally, but index.ts (notice the js vs the ts) on plunker. Not sure the best way to solve that. Seems like something the plunker builder would have to handle.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 17, 2016

I see what you mean. I will give it a thought.

@jim-cooper

This comment has been minimized.

jim-cooper commented May 18, 2016

@Foxandxss I've made the edits you recommended and I think it's much better now, thanks for the suggestions. Let me know if you have any thoughts on the plunker issue.

.file shared
.children
.file hero.service.ts
.file hero.ts

This comment has been minimized.

@Foxandxss
@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 18, 2016

The hero-list is also a barrel, so it needs to export too :)

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 18, 2016

Could you also please fix the indentation? You are mixing spaces with tabs. We use 2 spaces.

chore(docs): revise makeExcerpt mixin, add makeProjExample mixin
Be more DRY when referencing examples and excerpts. E.g.,
```jade
+makeExcerpt('quickstart/ts/app/app.component.ts', 'import',
'app/app.component.ts (import)')
```
can now be just
```jade
+makeExcerpt('app/app.component.ts', 'import')
```
Defined new mixin for examples named `makeProjExample` using this new
scheme.
The original `makeExample` has been left untouched.

Applied new mixins to quickstart.

closes #1420
@jmcooper

This comment has been minimized.

jmcooper commented May 18, 2016

Not sure what you mean by the hero-list component. If you mean the heros.component, it is being exported in the barrel. Can you clarify? :)

docs(dart): update BASICS intro (#1410)
Also copy edited the TS version a bit.
@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 18, 2016

If we see the style-guide, each folder is a barrel. That means that app/heroes exports a barrel, app/heroes/hero-detail/ exports a barrel, app/heroes/shared another barrel, etc.

@jmcooper

This comment has been minimized.

jmcooper commented May 18, 2016

Ahh, I see what you're saying. You said hero-list is a barrel, I think you meant hero-detail. But I wasn't under the impression from the "whys" listed in the style guide that we were suggesting creating barrels for a single import, but rather to aggregate multiple imports. hero-detail is being aggregated at a higher level with the other imports, should we really create an index file for a single import?

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 18, 2016

We are promoting that on the style guide, but I will double check first.

@Foxandxss

This comment has been minimized.

Member

Foxandxss commented May 18, 2016

After a conversation, yes, we add that even if it is one export.

@googlebot

This comment has been minimized.

googlebot commented May 20, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment
@googlebot

This comment has been minimized.

googlebot commented May 20, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added CLA: no and removed CLA: yes labels May 20, 2016

@jmcooper

This comment has been minimized.

jmcooper commented May 20, 2016

Ok, I've added the barrel for hero-detail and made the other changes.

However, it looks like the Plunker change isn't quite right. It appears to be only updating the first package in the list from .js to .ts and leaving the others. Once that's fixed I'll rebase again to grab it.

Btw, sorry for the "Merge branch..." commit, accidentally did a pull instead of a rebase on that last one.

@jmcooper

This comment has been minimized.

jmcooper commented May 20, 2016

Also, not sure why the CLA changed to no. It's because of the commits from other authors that I pulled in. Was I not supposed to rebase those changes into my branch?

@PascalPrecht

This comment has been minimized.

Contributor

PascalPrecht commented May 20, 2016

@jmcooper you've pushed commits from other ppl.

You should rebase your changes so that they don't appear here.

@wardbell

This comment has been minimized.

Contributor

wardbell commented May 20, 2016

Jim ... I'll look soon. No you're not supposed to merge which is why it looks like you changed 177 files. You don't want to submit a PR with 42 commits either.

But we understand. Git is PITA. We'll get you straightened out.

@wardbell

This comment has been minimized.

Contributor

wardbell commented May 21, 2016

Closing because (a) it's in Jim's fork and I can't help, (b) it's a tangle of unrelated commits, and (c) I think I was able to straighten this out in an IdeaBlade fork of angular.io.

I believe the proper contents are in PR #1458

@wardbell wardbell closed this May 21, 2016

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