toddmotto / angularjs-styleguide

AngularJS styleguide for teams
https://ultimateangular.com
5.96k stars 700 forks source link

Update component template examples #147

Closed GabrielGil closed 2 years ago

GabrielGil commented 7 years ago

Component templates should be specified either on the property template or in templateUrl.

Currently the examples include a hypothetical templateUrl directly inside an object.

Reference: Angular Components.

toddmotto commented 7 years ago

I'm not sure what's wrong here, the current implementation is correct based on the code, if it's an HTML file it needs templateUrl.

GabrielGil commented 7 years ago

Exactly, @xavhan is right. In the original styleguide, the import is not assigned to a component object parameter. Neither template or templateUrl.

toddmotto commented 7 years ago

If you import a string, you do - an HTML file is not a string. Try out the live app that this repo's readme links to - it follows this guide and structure

On 19 Jan 2017, at 17:17, Xavier Haniquaut notifications@github.com wrote:

If you import content from html file you get HTML so you need to put it in the template property of the component object.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

toddmotto commented 7 years ago

Try the live app the style guide links to it uses templateUrl, for example: https://github.com/toddmotto/angular-1-5-components-app/blob/ES2015/src/app/components/auth/auth-form/auth-form.component.js

@toddmotto

On 19 Jan 2017, at 18:37, Gabriel notifications@github.com wrote:

Exactly, @xavhan is right. In the original styleguide, the import is not assigned to a component object parameter. Neither template or templateUrl.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

GabrielGil commented 7 years ago

Ok, now I see that on that example app you use ngTemplate loader as suggested on tooling. I was expecting a regular html loader. Maybe what mixed me up was using a tool that was just "to be considered".

toddmotto commented 7 years ago

Ah sorry! Let me work on making this clearer. Sorry for the confusion!

On 19 Jan 2017, at 20:52, Gabriel notifications@github.com wrote:

Ok, now I see that on that example app you use ngTemplate loader as suggested on tooling. I was expecting a regular html loader. Maybe what mixed me up was using a tool that was just "to be considered".

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

GabrielGil commented 7 years ago

In my opinion, it'd be more clear if either the guide removes the consideration of ngTemplate and indicates that it is actually being used, or adapting the examples to be valid without the ngTemplate loader. This would be:

 /* ----- todo/todo.component.js ----- */
import template from './todo.html';

 export const TodoComponent = {
  template,
  controller: class TodoComponent {
  constructor(TodoService) {
        'ngInject';
  }
  // [...]
}
toddmotto commented 7 years ago

Yeah, I'm only using it here as the guide is focused on ES6 etc, thus tooling will be provided. Will rework the example when I have some time :)

xavhan commented 2 years ago

@GabrielGil would you mind closing this PR? It is still appearing in Pull-Requests Mentioned and I cant hide that it triggers my OCDs :D

Thanks :D

GabrielGil commented 2 years ago

Sure!