Angular Plugin: Removed isolate scope, added onPin, onUnpin, onTop, onNotTop events.

[spaceribs]

I removed the isolate scope, it's only hooked up to initialization right now anyway, so it shouldn't need to be bound and you can now apply your own ng-controller on an element with headroom applied. The example I used this is was for hiding a subnavigation when the headroom was unpinned.

I also added in events, although there is probably a cleaner way to do this. this should fix #105

[JobaDiniz]

Much better indeed. I'd also created my own version of the directive, using transclusion, something like:

restrict: 'E',
            replace: true,
            transclude: true,
            template: '<div ng-transclude></div>',

I guess this would be great to have as well.

[JobaDiniz]

@spaceribs I don't think it is working, have you tested it? Passing classes parameters isn't working.

[spaceribs]

@JobaDiniz can you give me an example of how you're using it?

[JobaDiniz]

Does not work classes="{initial: 'slide', pinned: 'slide--reset', unpinned: 'slide--up'}"

Works

 angular.forEach(Headroom.options, function(value, key) {
                    if(key == 'classes' && attrs[key])
                        options[key] = angular.fromJson(attrs[key]);
                    else
                   options[key] = attrs[key] || Headroom.options[key];
                });

But then, we need to change to valid JSON classes='{"initial": "slide", "pinned": "slide--reset", "unpinned": "slide--up"}'

[JobaDiniz]

And fix the $destroy event name, as proposed here: https://github.com/WickyNilliams/headroom.js/pull/104

[JobaDiniz]

Here's my contribution: https://gist.github.com/JobaDiniz/c14280ca3d7f33ca4287

[WickyNilliams]

I'm not familiar enough with angular to understand the reason for the change (or the correctness of it tbh!). Can you give a demo of what this change adds to the feature set (e.g. your use case in the description, hiding the subnav, would be good).

What about the issues @JobaDiniz identified (also should probably remove that console.log :)?

[JobaDiniz]

The callbacks (onPin, onTop, etc) should be bindable, so that's essentially what @spaceribs commit is doing.

@WickyNilliams I could create a PR with my contribution. What I did, besides improving @spaceribs code, was to add transclusion to the directive, which makes much more sense if you think about. With transclusion, I'm able to add html inside the headroom, so headroom function as a wrapper around some html content, like:

<headroom class="headroom container" offset="50" ng-disabled="shouldDisableHeadroom()" on-top="isOnTop = true" on-not-top="isOnTop = false" classes='{"initial": "slide", "pinned": "slide--reset", "unpinned": "slide--up"}'>
   <h1>My Content</h1>
</headroom>

And I think the directive should be used only like a html element, and never like an attribute (again, for me it just makes more sense).

Moreover, I add code to enable/disable the headroom, which in turn will call headroom.destroy() and headroom.init().

[aaron-bts]

yep, I would take @JobaDiniz code, I didn't test some of the features he checked but the main idea was that isolating the scope prevented anything from communicating outside of the directive, and in my case I wanted to communicate from ui-router into the header to close a menu in mobile.

[WickyNilliams]

Sorry for the delay all. Life got in the way.

I've read through this again, and still have no idea whether to accept. My angular skills are near zero, and there's so much angular-specific vocabulary (transclusion! directive! isolate scope!) that it's hard for me to understand. I may call on a friend with some angular experience to assist :)

[paul-barry-kenzan]

@JobaDiniz @WickyNilliams Any movement on this? PR says there are conflicts. Interested because I'm attempting to bind to onUnpin and haven't been able to get it working.

[toddmotto]

Do you want a review on this @WickyNilliams? Happy to refactor existing Angular PR to use $attrs to Observe potential changes. If we're using readonly properties then we can go down this route to use the $attrs Object. If you'd like to be able to $watch for changes on those parsed scope: {} values then we should integrate $watch where appropriate and register against the relevant callbacks in the Headroom instance.

[WickyNilliams]

@toddmotto what's the deal with this now that your other angular PR has been merged? What @JobaDiniz was saying above, about it being an element and not an attribute seemed to make sense. But all this angular stuff confuses the hell out of me :P

[toddmotto]

Well, the PR is pretty old now - ideally we should use isolate scopes rather than full inheritance. Happy to take some of the existing code and make a new PR, we'd need to consider what you want data-bound versus delegated functions. If you're delegating functions we should use scope: { fnName: '&' } to do this to allow callbacks inside the Directive. What functions do you need to keep alive in the Angular event loop? I'm not familiar with the unpin etc. methods so unsure which methods are more observers that respond to change and what are one-time calls. If you want to open an issue we can define the spec in there and look at approaching it with the currently merged Directive.

[spaceribs]

@toddmotto agreed, adding something like on-unpin="&" etc. to the directive is probably the best/most optimized option.

[WickyNilliams]

OK cool. I'm done for today, I'll open an issue tomorrow, and ping you guys. 👍