search

mozilla-magnet / magnet-client

A nearby content discovery client for Android & iOS
Mozilla Public License 2.0
19 stars 10 forks source link

Notifications service extension #342

Closed arcturus closed 8 years ago

arcturus commented 8 years ago

Now loading images for notifications.

@wilsonpage this PR includes the commits for the previous PR with new geofencing.

r?

arcturus commented 8 years ago

You can forget the first 3 commits, and check just the last one.

wilsonpage commented 8 years ago

@arcturus nice work! Some questions inline. How can I test the patch on device/simulator?

arcturus commented 8 years ago

For testing the patch you need to apply it, well, once I rebased it and when you have a notification pull down to get the expanded view.

arcturus commented 8 years ago

Just rebased the patch should be ready to test, here is a video showing it: https://www.youtube.com/watch?v=W35CylZbhZY

wilsonpage commented 8 years ago

Just rebased the patch should be ready to test, here is a video showing it: https://www.youtube.com/watch?v=W35CylZbhZY

'Video is private'

wilsonpage commented 8 years ago

For testing the patch you need to apply it, well, once I rebased it and when you have a notification pull down to get the expanded view.

I can use the simulator to mock location and then what? How do I get the notification to be dispatched?

arcturus commented 8 years ago

I can use the simulator to mock location and then what? How do I get the notification to be dispatched?

Yes, launch magnet, go to the background with the app, and change the location to the office 📦

wilsonpage commented 8 years ago

Looking at the video there is a blank space for a significant period of time until the image loads. Would it be much work to put a stock IOS loading spinner in place until the image has loaded?

arcturus commented 8 years ago

Looking at the video there is a blank space for a significant period of time until the image loads. Would it be much work to put a stock IOS loading spinner in place until the image has loaded?

I can try to add it, don't know how long it will take. I propose to work today on that, if I have it ready for the end of today the better, if not, we can land this and add a follow up.

Did you try on the phone?

arcturus commented 8 years ago

Hei @wilsonpage finally added the spinner, if the network is super fast we wont even see it, other wise I added a light dark background to the notification with a white spinner until we load the image and we replace spinner for image.

wilsonpage commented 8 years ago

Managed to test in simulator, works real nice, wicked!

I submitted a PR against your patch to fix some styling issues to do with spinner color and alignment.

wilsonpage commented 8 years ago

r+ after that :)

wilsonpage commented 8 years ago

NIT: In Android we call these 'ImageNotification's (instead of HeroNotificaiton) might be nice to have consistency in naming here. Took me a while to work out what 'hero' meant :)

arcturus commented 8 years ago

I'll open a follow up to rename that, although concepts are not the same, as this is a new target, but definitely I think you are right.