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

@team => Artsy Watch App #302

Merged
merged 9 commits into from Apr 1, 2015

Conversation

Projects
None yet
5 participants
@orta
Member

orta commented Mar 24, 2015

This one will take a while. And it is not my highest priority at all ATM. But given the API is public, I'm happy to have this out in some form, and can be worked towards a merge.

But as of right now this compiles and does the least amount of damage to Eigen at master. The watch app does not yet compile. Given the months + repo differences, I'm OK with this as a starting point.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy
Member

alloy commented Mar 24, 2015

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Mar 24, 2015

Member

The idea is to merge it as soon as possible so that we can get back to safely changing the Xcode project without introducing more merge conflicts, right?

Member

alloy commented Mar 24, 2015

The idea is to merge it as soon as possible so that we can get back to safely changing the Xcode project without introducing more merge conflicts, right?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Mar 24, 2015

Member

Yes, but I'd like the watch app to at least compile and be runnable. More importantly in order for this to work I have to make changes to our keychain groups so it's relallllllllllly important that we don't log people out encase of this.

Member

orta commented Mar 24, 2015

Yes, but I'd like the watch app to at least compile and be runnable. More importantly in order for this to work I have to make changes to our keychain groups so it's relallllllllllly important that we don't log people out encase of this.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Mar 24, 2015

Member

So maybe we can hold out until the current build is push to the store, then merge, then we can do Sync etc too. We could do it in our 3rd week of the #221 planning, anyone know when that is?

Member

orta commented Mar 24, 2015

So maybe we can hold out until the current build is push to the store, then merge, then we can do Sync etc too. We could do it in our 3rd week of the #221 planning, anyone know when that is?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Mar 24, 2015

Member

Looks like that day is tomorrow, so maybe this'll get merged by friday. Will aim for then.

Member

orta commented Mar 24, 2015

Looks like that day is tomorrow, so maybe this'll get merged by friday. Will aim for then.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Mar 26, 2015

Member

Groovy 👍

Member

alloy commented Mar 26, 2015

Groovy 👍

@orta

This comment has been minimized.

Show comment
Hide comment
@orta
Member

orta commented Mar 29, 2015

screen shot 2015-03-29 at 6 10 50 pm

@orta orta changed the title from [WIP] @team => Initial work on the watch merge to @team => Initial work on the watch merge Mar 29, 2015

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Mar 29, 2015

Member

OK, this is un-WIP'd - I would like people to look.

@1aurabrown this also includes changes around authentication, I have moved our Xapp token to a default, instead of keychain. I would like you to go over the changes in the ARUserManager.

@ashfurrow @alloy give it a one over, I know you won't know much about how the watch works ( and there's a lot of storyboard context missing too ) but I'd like to get this merged once we've deployed this sprint's release.

Member

orta commented Mar 29, 2015

OK, this is un-WIP'd - I would like people to look.

@1aurabrown this also includes changes around authentication, I have moved our Xapp token to a default, instead of keychain. I would like you to go over the changes in the ARUserManager.

@ashfurrow @alloy give it a one over, I know you won't know much about how the watch works ( and there's a lot of storyboard context missing too ) but I'd like to get this merged once we've deployed this sprint's release.

});
[self.artistNameLabel setText:remoteNotification[@"aps"][@"artwork_artist"]];
[self.artworkTitleLabel setText:remoteNotification[@"aps"][@"artwork_title"]];

This comment has been minimized.

@alloy

alloy Mar 30, 2015

Member

Very setter 😉

@alloy

alloy Mar 30, 2015

Member

Very setter 😉

This comment has been minimized.

@orta

orta Mar 30, 2015

Member

property syntax wasn't working for the first few betas, should be working now

@orta

orta Mar 30, 2015

Member

property syntax wasn't working for the first few betas, should be working now

@@ -0,0 +1 @@
Did you know that git does not support storing empty directories?

This comment has been minimized.

@alloy

alloy Mar 30, 2015

Member

Yeah, ‘git tracks content’, ‘Core Data is not an ORM’, yadda yadda… 💤

There’s a pattern of adding .gitkeep files to such directories.

@alloy

alloy Mar 30, 2015

Member

Yeah, ‘git tracks content’, ‘Core Data is not an ORM’, yadda yadda… 💤

There’s a pattern of adding .gitkeep files to such directories.

This comment has been minimized.

@orta

orta Mar 30, 2015

Member

I didn't add this, I think apple did.

@orta

orta Mar 30, 2015

Member

I didn't add this, I think apple did.

This comment has been minimized.

@alloy

alloy Mar 30, 2015

Member

lol

@alloy
MKCoordinateRegion region = MKCoordinateRegionMake(location, MKCoordinateSpanMake(miles/69.0, miles/(scalingFactor * 69.0)));
// Data from Artsy can be wrong, this causes an exception in the map preview
// so hide the map if we can't show it.

This comment has been minimized.

@alloy

alloy Mar 30, 2015

Member

Interesting, is the incorrectness documented somewhere and can you add a link to it?

@alloy

alloy Mar 30, 2015

Member

Interesting, is the incorrectness documented somewhere and can you add a link to it?

This comment has been minimized.

@orta

orta Mar 30, 2015

Member

I believe this is because we let our partner's put in their own addresses, and this doesn't always run through whatever GeoLocator we use correctly.

@orta

orta Mar 30, 2015

Member

I believe this is because we let our partner's put in their own addresses, and this doesn't always run through whatever GeoLocator we use correctly.

This comment has been minimized.

@orta

orta Mar 30, 2015

Member

I guess @dzucconi may know more?

@orta

orta Mar 30, 2015

Member

I guess @dzucconi may know more?

This comment has been minimized.

@dzucconi

dzucconi Mar 30, 2015

https://github.com/artsy/gravity/blob/77150c7fe14f61d36630a336c38a14075450fe24/app/models/domain/partner_location.rb#L127 is probably what you're interested in—that gets geocoded using the MapQuest API (https://github.com/artsy/gravity/blob/77150c7fe14f61d36630a336c38a14075450fe24/doc/Geocoding.md) + the coordinates can also be manually edited I believe. I don't know much more than that though.

@dzucconi

dzucconi Mar 30, 2015

https://github.com/artsy/gravity/blob/77150c7fe14f61d36630a336c38a14075450fe24/app/models/domain/partner_location.rb#L127 is probably what you're interested in—that gets geocoded using the MapQuest API (https://github.com/artsy/gravity/blob/77150c7fe14f61d36630a336c38a14075450fe24/doc/Geocoding.md) + the coordinates can also be manually edited I believe. I don't know much more than that though.

This comment has been minimized.

@alloy

alloy Mar 30, 2015

Member

@dzucconi Thanks!

@orta Worth it to quote @dzucconi’s comment in the code?

@alloy

alloy Mar 30, 2015

Member

@dzucconi Thanks!

@orta Worth it to quote @dzucconi’s comment in the code?

@property (readonly, nonatomic, strong) id referenceObject;
/// What is this message about
@property (readonly, nonatomic, assign) enum ARWatchMessageRequest action;

This comment has been minimized.

@ashfurrow

ashfurrow Mar 30, 2015

Member

If you typedef the NS_ENUM, then you can get rid of the enum keyword here.

@ashfurrow

ashfurrow Mar 30, 2015

Member

If you typedef the NS_ENUM, then you can get rid of the enum keyword here.

- (NSDictionary *)dictionaryRepresentation;
{
/// NSNulls cannot be sent through, so absence can be presumed by -1

This comment has been minimized.

@ashfurrow
@ashfurrow

This comment has been minimized.

@orta

orta Mar 30, 2015

Member

you're telling me

@orta

orta Mar 30, 2015

Member

you're telling me

This comment has been minimized.

@ashfurrow

ashfurrow Mar 30, 2015

Member

I'm going to go ahead and assume this limitation was well-documented?

@ashfurrow

ashfurrow Mar 30, 2015

Member

I'm going to go ahead and assume this limitation was well-documented?

This comment has been minimized.

@orta

orta Mar 30, 2015

Member

hah

@orta

orta Mar 30, 2015

Member

hah

_artwork = context;
// No viewDidLoad vs viewWillAppear:
if (self.loaded) return;

This comment has been minimized.

@ashfurrow

ashfurrow Mar 30, 2015

Member

This is super-annoying. They haven't fixed this in updated betas, have they?

@ashfurrow

ashfurrow Mar 30, 2015

Member

This is super-annoying. They haven't fixed this in updated betas, have they?

[self.thumbnail ar_asyncSetImageURL:self.show.thumbnailImageURL];
CLLocationCoordinate2D location = self.show.location.coordinate;
CGFloat miles = 3;

This comment has been minimized.

@ashfurrow

ashfurrow Mar 30, 2015

Member

booooo hissssss

@ashfurrow

ashfurrow Mar 30, 2015

Member

booooo hissssss

@ashfurrow

This comment has been minimized.

Show comment
Hide comment
@ashfurrow

ashfurrow Mar 30, 2015

Member

Looks good!

Member

ashfurrow commented Mar 30, 2015

Looks good!

orta added some commits Mar 30, 2015

@orta orta changed the title from @team => Initial work on the watch merge to @team => Artsy Watch App Mar 30, 2015

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Apr 1, 2015

Member

OK

It's happening.

Member

orta commented Apr 1, 2015

OK

It's happening.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Apr 1, 2015

Member

me, after hitting the button

Member

orta commented Apr 1, 2015

me, after hitting the button

orta added a commit that referenced this pull request Apr 1, 2015

@orta orta merged commit 55416e1 into master Apr 1, 2015

1 check was pending

continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@alloy alloy deleted the orta-watch branch Apr 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment