Skip to content

feat(TemplateLoader): cssUrl#580

Closed
PatrickJS wants to merge 2 commits into
angular:masterfrom
PatrickJS:css-url
Closed

feat(TemplateLoader): cssUrl#580
PatrickJS wants to merge 2 commits into
angular:masterfrom
PatrickJS:css-url

Conversation

@PatrickJS

@PatrickJS PatrickJS commented Feb 9, 2015

Copy link
Copy Markdown
Contributor

I noticed this feature in the docs but didn't work so I went ahead and implemented
it. The only problem I have atm is the createTemplateWithCss helper
method that should be either in dom or just in closure scope. I also need some
tests after your feedback about where to put the helper
createTemplateWithCss

@Component({
  selector: 'todo-header',
  componentServices: [
    AngularFire,
    bind(Firebase).toValue(new Firebase('https://webapi.firebaseio.com/test'))
  ],
  template: new TemplateConfig({
    url:    'app/components/todo-header/todo-header.html',
    cssUrl: 'app/components/todo-header/todo-header.css',
    directives: [
      Autofocus
    ]
  })
})

Edit: I could use inline styles below

<header>
  <h1>todos</h1>
  <input
    placeholder="What needs to be done?"
    [style]="styles"
    [value]="text"
    (keyup)="enterTodo($event)"
    autofocus
  >
</header>
var keymap = {
  tab: 9,
  enter: 13,
  esc: 27,
  up: 38,
  down: 40
};

@Component({
  selector: 'todo-header',
  componentServices: [
    AngularFire,
    bind(Firebase).toValue(new Firebase('https://webapi.firebaseio.com/test'))
  ],
  template: new TemplateConfig({
    url:    'app/components/todo-header/todo-header.html',
    cssUrl: 'app/components/todo-header/todo-header.css',
    directives: [
      Autofocus
    ]
  })
})
export class TodoHeader {
  text: string;
  todoService: FirebaseArray;

  constructor(sync: AngularFire) {
    // TODO: refactor into TodoStorage service
    this.todoService = sync.asArray();
    this.text = '';
    this.styles = {
      position: 'absolute',
      top: '-155px',
      width: '100%',
      fontSize: '100px',
      fontWeight: '100',
      textAlign: 'center',
      color: 'rgba(175, 47, 47, 0.15)',
      textRendering: 'optimizeLegibility'
    };
  }

  enterTodo($event) {
    // ENTER_KEY
    if ($event.which === keymap.enter) {
      // if value
      if ($event.target.value !== '') {
        this.text = $event.target.value;
        this.addTodo(this.text);
      }
    }
  }

  addTodo() {
    this.todoService.add({
      title: this.text,
      completed: false
    });
    this.text = '';
  }

}

@PatrickJS
fix(dom): correctly set css for style
1c0e746
@PatrickJS
feat(TemplateLoader): cssUrl
7af7171 
I noticed this feature but didn’t work so I went ahead and implemented
it. The only problem I have atm is the createTemplateWithCss helper
method that should be in dom or just in closure scope. I also need some
tests after your feedback about where to put the helper
createTemplateWithCss.
@Hendrixer

Hendrixer commented Feb 9, 2015

Copy link
Copy Markdown

👍

@pkozlowski-opensource

pkozlowski-opensource commented Feb 9, 2015

Copy link
Copy Markdown
Member

There were no final, written conclusion from the discussion on a similar PR. But I was under the impression that CSS urls don't really belong to TemplateConfig and should rather be parsed out from the template itself.

//cc: @mhevery

@PatrickJS

PatrickJS commented Feb 9, 2015

Copy link
Copy Markdown
Contributor Author

it looks like I did what Miško suggested by inserting <style> inside the people in react world would use the inline style attribute or in ng1 its ng-style so I could just use that with a style Decorator

@pkozlowski-opensource

pkozlowski-opensource commented Feb 9, 2015

Copy link
Copy Markdown
Member

@gdi2290 depends on how you understand "template": on a TemplateConfig level, that is in JS (as you change seem to do) or inside HTML template fragment (for example, inside <style> HTML tag) so on the HTML level.

IMO conceptually CSS should be tied to HTML of a template and not a component's JS and this is how I'm interpreting comments in the issue I've linked to.

@PatrickJS

PatrickJS commented Feb 9, 2015

Copy link
Copy Markdown
Contributor Author

I agree css should be in html but then again a few years back someone could say that about javascript in html. Facebook/React/Pete Hunt are pushing for css to be managed js and explains all the benefits from doing so. One hack Famo.us, with the angular adapter, had to do was providing callbacks in js for css classes in order to manage css correctly. Knowing that I'm personally on the side of css being managed by js

@pkozlowski-opensource

pkozlowski-opensource commented Feb 9, 2015

Copy link
Copy Markdown
Member

The obvious downside being that one can't simply swap a template + CSS for a given component, which is pretty bad IMO. Anyway, let's see what others got to say...

@PatrickJS

PatrickJS commented Feb 9, 2015

Copy link
Copy Markdown
Contributor Author

I added my style decorator for more feedback on how people should handle styles in ng2 #583

@mhevery

mhevery commented Feb 10, 2015

Copy link
Copy Markdown
Contributor

@gdi2290 I love your enthusiasm! Could you check with me on features to make sure we don't dulpicate effort? As it stands right now we have chose to put <style> tag int HTML rather than cssURL here. This has to do with it being more in line with WebComponents, so I fear we can't accept your PR in the current state. See https://github.com/angular/angular/pull/560/files for the current implementation of this.

@mhevery

mhevery commented Feb 11, 2015

Copy link
Copy Markdown
Contributor

I am going to close this as we are not planning to solve this in this way. We will certainly revisit if we change our mind.

@mhevery mhevery closed this Feb 11, 2015
@PatrickJS

PatrickJS commented Feb 12, 2015

Copy link
Copy Markdown
Contributor Author

no worries, it was a great exercise getting it to work correctly

@PatrickJS PatrickJS deleted the css-url branch February 12, 2015 07:20
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 6, 2019

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PatrickJS @Hendrixer @pkozlowski-opensource @mhevery @googlebot