Skip to content

New Reader list design#575

Merged
mikejohnstn merged 39 commits intodevelopfrom
feature/485-new-reader
Nov 19, 2013
Merged

New Reader list design#575
mikejohnstn merged 39 commits intodevelopfrom
feature/485-new-reader

Conversation

@mikejohnstn
Copy link
Contributor

This is the new design implemented for #485: layout, typography, padding, buttons, and colors. It includes many aesthetic changes as well as an overall code cleanup and refactoring. It only changes the list view, not the detail view.

There are some known shortcomings:

  • The comments button does nothing yet. This will change once comments are unified in another pull request.
  • You can browse to a tag, but this experience isn't great yet; improvements are outside of this scope.
  • The experience of scrolling remains imperfect (ideally images would almost always load off-screen, but this doesn't happen); also outside of this scope.
  • The iPad experience could be much more beautiful and deserves its own design in the future.

Screenshot (iPhone):
newreader-iphone

Screenshot (iPad):
newreader-ipad

Video:
https://cloudup.com/cNo8p0iztTA

- Read tag details off the wire, parse, store them
- Display primary tag along with each post
- Fix various height calculations
- Also handle sites that can't be followed
Copy link
Member

Choose a reason for hiding this comment

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

This needs translation support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was already in the app. I just moved it to a category. I've added #583 to localize this and ensure it makes sense for all supported languages.

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 a pretty good rule of thumb that if you see 86400 in code, it's not the best way to calculate duration in days. Where you'll run into problems is with people using other than a Gregorian calendar or when daylight saving and leap years/seconds come into play.

I'd suggest using NSDateComponents (this code is typed from memory and may not be compilable) -

NSCalendar *calendar = [NSCalendar currentCalendar];
NSUInteger unitFlags = NSDayCalendarUnit | NSHourCalendarUnit | NSMinuteCalendarUnit | NSSecondCalendarUnit;

NSDateComponents *components = [gregorian components:unitFlags fromDate:startDate toDate:endDate options:0];

// then

NSInteger days = [components day];
NSInteger hours = [components hour];
//etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't write this (just moved it), but yeah, to do it 100% right, someone will need to research if this rendering of dates even makes sense in all the languages we support. It's a bigger task that I think we should defer to #583 (this code already exists in the production app).

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops I didn't see #583 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Three NSString attributes were added to ReaderPost.

Conflicts:
	WordPress/Classes/ReaderPostTableViewCell.m
@mikejohnstn
Copy link
Contributor Author

Re-added colorWithHexString: method to UIColor+Helpers as part of that last merge.

@astralbodies
Copy link
Contributor

This looks good with the only comment being the time calculation.

Lets make sure that we spend an amount of time with testing the Core Data migration as well from 12-13.

:shipit:

mikejohnstn added a commit that referenced this pull request Nov 19, 2013
@mikejohnstn mikejohnstn merged commit c940921 into develop Nov 19, 2013
@mikejohnstn mikejohnstn deleted the feature/485-new-reader branch November 19, 2013 20:51
@koke
Copy link
Member

koke commented Nov 21, 2013

@astralbodies
Copy link
Contributor

@koke - nice! We use the NSInferMappingModelAutomaticallyOption so I was more concerned about people upgrading from 11 to 13. I don't think Core Data is smart enough to apply the mapping model to go from 11 to 12 and then infer the mapping for 12 to 13.

astralbodies added a commit that referenced this pull request Dec 6, 2013
…ssComApi.h in #575 PR #692.  This should be part of #22 and checked into develop when the warnings are resolved
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.

3 participants