-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial implementation of directives; include implemented. #55
Conversation
# Extend the options with the name and namespace so that the | ||
# compiler has these configuration properties from the beginning | ||
# and that the API for this method can stay the same. | ||
options.namespace = namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about deprecating these params and just ask for (source, options = {})
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer that; thanks for mentioning it. I meant to mention it in the post. It's something I was going to ask. @netzpirat what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the change, but this function is part of the public API and we need to make sure that it's done in a backward compatible way, so that both function signatures are supported.
Thanks a lot for the initial implementation. I just returned from holidays and will try it within the next few days and have also a look on possible testing strategies for cases where multiple templates are involved. |
FYI, I didn't forget this, I'm just feeling sick and I'm not able to dig into this right now. |
@netzpirat Take your time. Hope you feel better soon. |
Just extended the spec suite to setup globally placed partials, which makes integration tests for the |
🍰 |
@Nami-Doc I added you also as contributor. I try to follow an open commit bit policy on my repositories like @jfirebaugh |
not sure why but thanks I suppose :p |
@Nami-Doc Because you've made a pull request that was accepted. |
@Nami-Doc Great read, thanks for sharing. |
I've started work on the directives. I've got the first bit done (having the compiler recognize directives) and implemented include. I figure it's better to do this in pieces as #50 is a bit large.
The implementation works nicely (at least in my testing) for placement in global as well as AMD (it could work for standalone too but I can add that after that PR is merged I suppose).
I attempted to follow your existing conventions around the code base when adding directives; let me know where the implementation could be improved, etc.
Also... I couldn't figure out how to get jasmine to unit test the include directive as two templates need to be compiled and one rendered and your testing harness only allows for one and one. Should I add more specs or do you have another idea?