Added onChange event, with proper cleanup (on $destroy)

[sravenhorst]

• onChange event, which ONLY files when switching between mediaqueries in the rules.
• requires the scope for proper cleanup (on $destroy).
• Moves some code around.

Some additional comments to be considered changing:

• Proper cleanup (on $destroy) should also be done for the on function.
• binding multiple times to .on() will create multiple listeners to the window onresize event, would be nice to bring this back to always have max 1 eventlistener.

[MattSidor]

+1, I've started using this in production and it works great. @jacopotarantino, do you think you will pull this into the main repo?

[dderiso]

Just found this service and it looks useful. Would be great if these were pulled!

[jacopotarantino]

Sorry for the slow followup on this. Pretty sure I have a tab open somewhere to properly review this. In the mean time, I feel comfortable merging it in knowing that somebody else is using it and has seen it working. Thanks @sravenhorst!

[MattSidor]

Thanks @jacopotarantino and @sravenhorst!

[jacopotarantino]

Version 0.5.0 which contains this update is now available tagged and via bower.

[sravenhorst]

Great! Thanks for merging and happy i could help people! :-)

[MattSidor]

@sravenhorst and @jacopotarantino, I owe you guys an apology for not thoroughly testing these changes. I pulled 0.5.0 and it's not working for me now.

Strangely I thought it was working yesterday when I manually linked to sravenhorst's fork but maybe I did not properly refresh my dev environment and it was using a cached version.

For my production environment I am reverting to 0.4.1 and I will try to debug this pull request over the weekend. Again sorry for pushing you guys to merge this so quickly without testing it more thoroughly.

[sravenhorst]

Hi!

Too bad to hear things don't work for you. I use my fork/branch in our production environment, which has quite some users, and for me it's working great.

What doesn't work exactly? Do you've issues with the onChange function? Or is "on" not working anymore?

I'll look into this asap if you can give me more details.

Cheers, Sander

[MattSidor]

I'm using "on" and that is not working anymore:

screenSize.rules = {
    retina: 'only screen and (min-device-pixel-ratio: 2), only screen and (min-resolution: 192dpi), only screen and (min-resolution: 2dppx)',
    lg: '(min-width: 1200px)',
    md: '(min-width: 992px) and (max-width: 1199px)',
    sm: '(min-width: 768px) and (max-width: 991px)',
    xs: '(max-width: 767px)'
};

$scope.desktop = screenSize.on('md, lg', function(match){
    $scope.desktop = match;
});
$scope.mobile = screenSize.on('xs, sm', function(match){
    $scope.mobile = match;
});
$scope.retina = screenSize.on('retina', function(match){
    $scope.retina = match;
});

I have reverted to commit c117019 in your fork, Sander, and it's not working there either. I think I must have had a cached version in my browser when I thought it was working before.

I have several other angular-related libraries that might be causing a conflict, so I need to test this in a simplified version with just angular and angular-match-media. That's what I'll be doing next.

[MattSidor]

Okay, I've made a simple test plunkr and specifically I am having a problem detecting retina displays in the new version.

$scope.retina is incorrectly false in version 0.5.0, but correctly true in version 0.4.1.

Plunkr with version 0.5.0: http://plnkr.co/edit/isuDkgC6Sc2NlzXwsZ5B?p=preview Plunkr with version 0.4.1: http://plnkr.co/edit/r62JOQxcZhBTAAGXP2UA?p=preview

Seems to be detecting display widths correctly, it's just the retina rule that is no longer working.

[sravenhorst]

Thanks for your plunker!

I'll debug the code right away.

[jacopotarantino]

I really appreciate this effort guys, thanks.

[sravenhorst]

Quick question @seadour: What exactly do you use the retina check for?

In my thoughts there can be only 1 valid rule at the time, for example xs or sm. Thats why i simplified some logic. But with retina added to your rules there can now be multiple valid rules, which kills the code i changed.

Retina has not much to do with the screensize imo, the screen is retina or not. What are your thoughts about adding just an extra public prop "isRetina"?

I also can't think of an use case for .on(retina), as your pixel density will never change, but maybe i'm not thinking clear or missing something :-)

Like to hear from you!

Cheers, Sander

[MattSidor]

I check for retina to pull higher-resolution images for our app. We have two sets of images: "standard" resolution, and 2x "retina" resolution. I just use this check to choose between the two versions.

Pixel density can change if you have 2 or more monitors set up, and they are of varying pixel densities. My current work setup is this way -- I have a MacBook Pro (retina) with its display extended to a generic LCD display (non-retina). If I move my browser window between the two displays, .on(retina) could be useful here.

However I do think an extra "isRetina" prop would be fine for my needs, as I don't really need to be detecting pixel density changes while my apps are open; the use case I just outlined in the previous paragraph is probably a really uncommon situation and I think developers can just look for pixel density on the initial app load.

On Mon, Dec 21, 2015 at 1:32 PM Sander Ravenhorst notifications@github.com wrote:

Quick question @seadour https://github.com/seadour: What exactly do you use the retina check for?

In my thoughts there can be only 1 valid rule at the time, for example xs or sm. Thats why i simplified some logic. But with retina added to your rules there can now be multiple valid rules, which kills the code i changed.

Retina has not much to do with the screensize imo, the screen is retina or not. What are your thoughts about adding just an extra public prop "isRetina"?

I also can't think of an use case for .on(retina), as your pixel density will never change, but maybe i'm not thinking clear or missing something :-)

Like to hear from you!

Cheers, Sander

— Reply to this email directly or view it on GitHub https://github.com/jacopotarantino/angular-match-media/pull/22#issuecomment-166430530 .

[sravenhorst]

ah ok! Thanks for info.

I forked your plunker and fixed the issues on the fly, could you test if this works for you?

It now has isRetina.

http://plnkr.co/edit/mqaURfDH51UJs5RDMwqW?p=preview

I did find 1 or 2 other issues with onChange, which i fixed as well, of of them was that specifyig the following didn't work as expected:

$scope.isMobile = screenSize.onChange($scope, 'xs,sm', function(isMatch){
  console.log('change triggered', isMatch);
}

That code would also trigger the change event between xs & sm, also fixed in that plunker.

@jacopotarantino: How do you feel about adding isRetina as a seperate prop to the screenSize service? I could see that that could also go into a seperate angular service, but it is kind of nice to have it all together in your service.

[MattSidor]

Looks perfect. Thanks Sander.

On Mon, Dec 21, 2015 at 2:42 PM Sander Ravenhorst notifications@github.com wrote:

ah ok! Thanks for info.

I forked your plunker and fixed the issues on the fly, could you test if this works for you?

It now has isRetina.

http://plnkr.co/edit/mqaURfDH51UJs5RDMwqW?p=preview

I did find 1 or 2 other issues with onChange, which i fixed as well, of of them was that specifyig the following didn't work as expected:

$scope.isMobile = screenSize.onChange($scope, 'xs,sm', function(isMatch){ console.log('change triggered', isMatch); }

That code would also trigger the change event between xs & sm, also fixed in that plunker.

@jacopotarantino https://github.com/jacopotarantino: How do you feel about adding isRetina as a seperate prop to the screenSize service? I could see that that could also go into a seperate angular service, but it is kind of nice to have it all together in your service.

— Reply to this email directly or view it on GitHub https://github.com/jacopotarantino/angular-match-media/pull/22#issuecomment-166444217 .

[sravenhorst]

great to hear!

I'll test the changes a bit more thoroughly tomorrow ( it's 00:25 here atm :-) ) on my companies corporate website to double check that all usages are working as expected, I'll create a pull request afterwards.

Cheers

[jacopotarantino]

@sravenhorst I'm pretty okay with the isRetina method. I have concerns about naming it that instead of "hi-dpi" or something more brand-neutral but i can't think of anything that makes sense and people would recognize. Could you PR your changes and make any necessary modifications to the Readme?

[dderiso]

This is awesome, thanks for all the updates!!

[sravenhorst]

Thanks for your feedback @jacopotarantino! Unfortunately i wasn't able to pick this up today, will do tomorrow.

Sander