Skip to content
This repository was archived by the owner on Dec 4, 2017. It is now read-only.

docs(guide): add dart version of hierarchical dependency injection #683

Closed

Conversation

andresaraujo
Copy link
Contributor

Adds Dart version of hierarchical dependency injection guide.

@kwalrath Could you take a look?

Thanks.

@Output() EventEmitter canceled = new EventEmitter();
@Output() EventEmitter saved = new EventEmitter();

RestoreService<Hero> _restoreService;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one can be final

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also private. Good catch +1
Edit: nope it must be public.

- angular2:
platform_directives:
- 'package:angular2/common.dart#CORE_DIRECTIVES'
- 'package:angular2/common.dart#FORM_DIRECTIVES'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't juts #COMMON_DIRECTIVES cover both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yjbanov what would you recommend?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CORE_DIRECTIVES only include:

const List<Type> CORE_DIRECTIVES = const [
  NgClass,
  NgFor,
  NgIf,
  NgStyle,
  NgSwitch,
  NgSwitchWhen,
  NgSwitchDefault
];

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend including the minimum necessary. It does not look like you need the form directives for this example app.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hero_editor_component.dart needs an NgModel, if I don't include FORM_DIRECTIVES It won't work.

Btw If I only include directives: const [NgModel] in that component it also does't work, only works with FORM_DIRECTIVES. Is this a bug or work as intended?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either a bug or possibly you forgot an import?

@andresaraujo
Copy link
Contributor Author

I think made all the corrections, I end up using: (Thanks to zoechi)

    platform_directives:
    - 'package:angular2/common.dart#COMMON_DIRECTIVES'

Do you want me to change this for a directive FORM_DIRECTIVES in the hero_editor_component.dart?

@kwalrath

@kwalrath
Copy link
Contributor

@andresaraujo I think it's better to keep everything in platform_directives.

I don't know whether it's better to use COMMON_DIRECTIVES or CORE_DIRECTIVES & FORM_DIRECTIVES. @yjbanov?

@kwalrath
Copy link
Contributor

This looks good to me, as far as I can tell. @sanfordredlich can you do a technical review?

@kwalrath
Copy link
Contributor

@andresaraujo: You don't need to squash commits when you upload. I'll take care of that when I merge, and it'd be nice to be able to more easily see the diffs between versions.

@sanfordredlich
Copy link

LGTM. Trivial: you might want to match case on the power:

  • new Hero()+ ..name = "RubberMan"+ ..power = 'flexibility',+ new Hero()+
    ..name = "Tornado"+ ..power = 'Weather changer'

On Tue, Jan 12, 2016 at 2:21 PM Kathy Walrath notifications@github.com
wrote:

@andresaraujo https://github.com/andresaraujo: You don't need to squash
commits when you upload. I'll take care of that when I merge, and it'd be
nice to be able to more easily see the diffs between versions.


Reply to this email directly or view it on GitHub
#683 (comment).

@yjbanov
Copy link

yjbanov commented Jan 13, 2016

@kwalrath

I don't know whether it's better to use COMMON_DIRECTIVES or CORE_DIRECTIVES & FORM_DIRECTIVES. @yjbanov?

I have no preference

@kwalrath
Copy link
Contributor

LGTM! I'll merge this.

@kwalrath kwalrath closed this in ec392ab Jan 13, 2016
kasperpeulen pushed a commit to kasperpeulen/angular.io that referenced this pull request Jan 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants