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.

Sign up for GitHub

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

Jump to bottom

GH-21 Meetup5 Improve Todo cell UI #26

Merged
merged 5 commits into from Nov 9, 2018
Merged

GH-21 Meetup5 Improve Todo cell UI #26

merged 5 commits into from Nov 9, 2018

Conversation

ivancr
Copy link
Collaborator

@ivancr ivancr commented Oct 2, 2018

  • Added completion date property to Item Model
  • Added an icon for complete and incomplete state.
  • Added label to display completed date
  • Setup labels layout to vertical when dynamic type size is large.
  • Removed autoresizing masks form xibs
  • Cleaned up code and added pragma marks

@ivancr
GH-21 Meetup 5 Improve Todo Cell UI:
04ff682 
- Added completion date property to Item Model
- Added an icon for complete and incomplete state.
- Added label to display completed date
- Setup labels layout to vertical when dynamic type size is large.
- Removed autoresizing masks form xibs
- Cleaned up code and added pragma marks
@ivancr
GH-21
3faceb2
@ivancr
Merge branch 'master' of https://github.com/chuva-io/TodoList into GH-21
17c090c 
-Meetup5-Improve-Todo-Cell-UI

# Conflicts:
#	TodoList/View Controllers/TodoListViewController.swift
@ivancr
Copy link
Collaborator Author

ivancr commented Oct 2, 2018

GH-20 PR needs to be merged before we can merge this one.

@nnascim nnascim requested review from ErickApps, nnascim and ryantstone and removed request for nnascim October 2, 2018 15:25
@nnascim
Copy link
Owner

nnascim commented Oct 2, 2018

GH-20 PR needs to be merged before we can merge this one.

Next time submit the pull request on the branch it depends on. You're requesting to merge it into master.

stemberamichal
stemberamichal reviewed Oct 2, 2018
View reviewed changes
TodoList/Models/Item.swift
@@ -2,10 +2,12 @@ import Foundation

struct Item: Codable {
let title: String
var completionDate: Date?
var isComplete: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect isComplete to be calculated property of completionDate.
This can introduce inconsistency between those two.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great point @stemberamichal. I'll change that when I get a chance. Thanks!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created an issue (GH-30) to deal with this. I don't want to introduce it in this PR that is focused on other topic.

stemberamichal
stemberamichal reviewed Oct 3, 2018
View reviewed changes
TodoList/View Controllers/TodoListViewController.swift
tableView.estimatedRowHeight = 44
tableView.rowHeight = UITableViewAutomaticDimension
tableView.estimatedSectionFooterHeight = 80
tableView.sectionFooterHeight = UITableViewAutomaticDimension
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you replace UITableViewAutomaticDimension?
I have looked through the changes and could not find it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UITableViewAutomaticDimension is set by default when done programmatically.

https://developer.apple.com/documentation/uikit/uitableview/1614852-rowheight

The default value of rowHeight is automaticDimension. Note that if you create a self-sizing cell in Interface Builder, the default row height is changed to the value set in Interface Builder

stemberamichal
stemberamichal reviewed Oct 3, 2018
View reviewed changes
TodoList/View Controllers/TodoListViewController.swift Outdated
tableView.rowHeight = UITableViewAutomaticDimension
tableView.estimatedSectionFooterHeight = 80
tableView.sectionFooterHeight = UITableViewAutomaticDimension
tableView.keyboardDismissMode = .interactive
Copy link
Contributor

Choose a reason for hiding this comment

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

That's so cool.
Have been using UIScrollViewDelegate to dismiss keyboard.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I love finding these tricks. I try to find the simplest way that uses the least code possible.

nnascim
nnascim previously approved these changes Oct 7, 2018
View reviewed changes
@ivancr
GH-20 Meetup5 Empty state (#25)
bddfeba 
* GH-21 Changes:
- Moved TableView to child VC.
- Handled Keyboard
- Added Empty state
- Fixed textField placeholder Dynamic Type support
- Added Extensions to add and remove child VC's, add subview with fill constraints
- Made Navbar opaque.

* GH-20 Empty state improvements
Merge branch 'master' of https://github.com/chuva-io/TodoList into GH-21
93a1af9 
-Meetup5-Improve-Todo-Cell-UI

# Conflicts:
#	TodoList/AppDelegate.swift
@ivancr ivancr merged commit f2da8d2 into master Nov 9, 2018
@ivancr ivancr deleted the GH-21-Meetup5-Improve-Todo-Cell-UI branch November 9, 2018 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stemberamichal stemberamichal stemberamichal left review comments

@ErickApps ErickApps ErickApps approved these changes

@nnascim nnascim nnascim left review comments

@ryantstone ryantstone Awaiting requested review from ryantstone

@gcharles-chwy gcharles-chwy Awaiting requested review from gcharles-chwy

Assignees

@ivancr ivancr

Labels
Meetup 5 UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ivancr @nnascim @stemberamichal @ErickApps