Skip to content
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

[WorksForYou] V1 of the view controller #1185

Merged
merged 15 commits into from
Feb 25, 2016
Merged

Conversation

mennenia
Copy link
Contributor

  • Hooked up to network model
  • Click on artwork pushes VC
  • Loads next works upon scrolling

TODO (on top of what's in #1158)

  • Correct spacing
  • Show load indicator when scrolling
  • Disable horizontal scrolling

Demo:

Scroll and load:
gif2

Click:
gif1

Will do this later today hopefully, and then separately do new snapshot tests, mark notification as read, etc

@ArtsyOpenSource
Copy link
Contributor

1 Warning
⚠️

Needs testing on a Phone if change is non-trivial

Generated by 🚫 danger

@orta
Copy link
Contributor

orta commented Feb 19, 2016

An easy rule to remember is that we never use Avant Garde with lowercase letters, so double check the designs WRT the title



@interface ARWorksForYouViewController : UIViewController
@interface ARWorksForYouViewController : UIViewController <AREmbeddedModelsViewControllerDelegate, UIScrollViewDelegate>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this information need to be public knowledge? e.g. could it of in the .m

@katarinabatina
Copy link

@mennenia tiny to do, not sure if you want to add it to this PR or not but the artist names should be all caps. This is looking awesome tho!

@mennenia
Copy link
Contributor Author

Yup that's been fixed in the label pod :) I know it's not reflected in the gif yet, sorry!

@mennenia
Copy link
Contributor Author

Okay, @orta I'm ready for this one to be reviewed/closed. I thought I did what I could to disable horizontal scrolling, but that isn't working at the moment. Will revisit.

Upcoming PRs: I will make the artist clickable (which will make the VC a bit nicer, as I'll take out some of the view code and put it in a separate class), sort out iPad (needs some better spacing), then snapshots.

Current looks:

@mennenia mennenia changed the title [WIP] [WorksForYou] V1 of the view controller [WorksForYou] V1 of the view controller Feb 25, 2016
[df setDateFormat:@"yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"];

return [df dateFromString:(NSString *)_publishedAt];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably better that you leave this to the view controller - I don't think an user of the model would expect the performance hit that would come with this, ( you can also use ARStandardDateFormatter to use the standard for setting formats )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I follow - the problem is that the type of "publishedAt" is an NSString, even though it's described as an NSDate. Basically, when getting it from the json, the string is put into the property without converting it into a date. If you want I can do this logic in the VC, but then the type should become "NSString *publisedAt" right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, then you need to create a value transformer for it, then it's done on the BG parsing thread, e.g.

+ (NSValueTransformer *)startDateJSONTransformer
{
    return [ARStandardDateFormatter sharedFormatter].stringTransformer;
}

+ (NSValueTransformer *)endDateJSONTransformer
{
    return [ARStandardDateFormatter sharedFormatter].stringTransformer;
}

I think you'd want:

+ (NSValueTransformer *)publishedAtJSONTransformer
{
    return [ARStandardDateFormatter sharedFormatter].stringTransformer;
}

@orta
Copy link
Contributor

orta commented Feb 25, 2016

Cool beans - this looks solid 10/10

orta added a commit that referenced this pull request Feb 25, 2016
[WorksForYou] V1 of the view controller
@orta orta merged commit 6a7738c into master Feb 25, 2016
@alloy alloy deleted the mc-version1-nativeforyouvc branch March 3, 2016 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants