Skip to content
This repository was archived by the owner on Sep 7, 2018. It is now read-only.

Annotation Improvement #10

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Annotation Improvement #10

wants to merge 8 commits into from

Conversation

Romakita
Copy link

@Romakita Romakita commented Mar 7, 2016

Hello ulfryk,
I'd just fix a bug on my code but I had to fix some bugs on your package.json and bower.json :

  • Your package doesn't work with Angular 1.5 (or 1.5 is bugged)
  • Your version of Karma is bugged too. Change version fix them.

I didn't write more unit test because I don't like CoffeeScript :) but a thinks you can write test. The added features isn't very complicated and you know this language better than me !

I think, the TSHint configuration is a best practice, but this configuration is too strict to encourage a developer to work on your component :/ (I waist 1 hour just for fix my with TSHint ... XD )

Best regards :)

Romain

})
class SomeDirectiveController {

public static link: angular.IDirectiveLinkFn = (scope, element, attrs, ctrl: SomeDirectiveController) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I think that static link member should be actually moved o@directive decorator in this example.

Copy link
Author

Choose a reason for hiding this comment

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

I think isn't a best practice to declare a function into an annotation. I never see that in Java.

But I think, effectivily, the description isn't clear...

Copy link
Owner

Choose a reason for hiding this comment

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

I think isn't a best practice to declare a function into an annotation.

Totally agree. That is why I decided to move the configuration to static members, not to annotation.

Problem is that this linking function must be in this example in annotation, or it will not work at all…

Copy link
Author

Choose a reason for hiding this comment

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

I think work fine because have these instructions (line 116 - 125) :

config = <IDirectiveProperties> angular.copy(directiveSettings);

// Store FuncName as ControllerAs
config.controllerAs = getFuncName(target);
config.controller = target;

angular.forEach(directiveProperties, function(property: string): any {
    if (angular.isDefined(target[property])) {
       config[property] = target[property];
    }
})

Link method will be stored in config object.

Copy link
Owner

Choose a reason for hiding this comment

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

You are totally right. I focused on a if (typeof directiveSettings === 'string') { guard and didn't notice the part in lines 122 - 126.

So my observation is invalid.

Now I see - there is another issue. Look down to those lines…

@Romakita
Copy link
Author

Hello Ulfryk,

Did you see my last update ?

@ulfryk
Copy link
Owner

ulfryk commented Mar 15, 2016

@Romakita - sorry for delay, I have a lot of hard work with other things. Just give me few days ;)

@@ -2,11 +2,11 @@
"name": "angular-typescript",
"version": "0.0.8",
"dependencies": {
"angular": "^1.2.0",
"angular-resource": "^1.2.0"
"angular": "1.4.0",
Copy link
Owner

Choose a reason for hiding this comment

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

1.5.2 maybe ?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe. 1.5 give me an error.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok leave it on 1.4 I'll fix errors and upgrade deps later.

@ulfryk
Copy link
Owner

ulfryk commented Mar 21, 2016

@Romakita - sorry for delay - just few last fixes :)

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.

2 participants