Skip to content

[FEAT] Implement donations controller - #107

Merged
Kilo-Loco merged 12 commits into
developmentfrom
implement-donations-controller
Jun 7, 2020
Merged

[FEAT] Implement donations controller#107
Kilo-Loco merged 12 commits into
developmentfrom
implement-donations-controller

Conversation

@Kilo-Loco

Copy link
Copy Markdown
Member

This PR implements almost everything needed for the Donations Controller

  • The filters at the top have been implemented in a reusable way
  • The donations collection view was built with a manager that uses diffable data source in a reusable way
  • Many other miscellaneous abstractions that allow cleaner, more readable code


Screen Shot 2020-06-06 at 4 11 29 PM

@nedimf nedimf left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Awesome job

@Kilo-Loco Kilo-Loco added draft review me This PR needs more reviewers and removed draft labels Jun 7, 2020

@dillon-mce dillon-mce left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm new to this project, so just looking over PRs to get a feel for everything. Looks like a lot of good work!

Comment on lines +25 to +29
extension UIStackView {
func addArrangedSubviews(_ views: UIView...) {
views.forEach { self.addArrangedSubview($0) }
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎉

Comment on lines +29 to +34
private static func dynamic(light: UIColor, dark: UIColor) -> UIColor {
return .init { trait in
return trait.isDarkModeOn ? dark : light
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't know that this is clearer than just using the initializer that you're wrapping. It is also kind of limited. What if you wanted to add colors for high contrast?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added this so we could at least start adding dynamic colors to the app so they work during dark mode. definitely a lot of room to improve here in a different PR

return label
}()
private var titleLabel = UILabel.create {
$0.translatesAutoresizingMaskIntoConstraints = false

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line and lines 61-66 were all accomplished in titleLabel.fillSuperView() (L50) before. Is the style you have here preferred? I assumed we should be using those convenience methods.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Did this to be consistent with the rest of the code in the diff but will add a quick follow up diff to use fillSuperView()

@Kilo-Loco
Kilo-Loco merged commit 99ea0c4 into development Jun 7, 2020
@Kilo-Loco
Kilo-Loco deleted the implement-donations-controller branch June 7, 2020 01:46
@Sharkesm Sharkesm changed the title Implement donations controller [FEAT] Implement donations controller Jun 7, 2020
hybridcattt added a commit that referenced this pull request Jun 7, 2020
…homePageDiffHeader

* commit '0849144d5263661a10facbfca254c978685cf7cc':
  [Chore] Add theme file (#85)
  [Chore] Refactor filter category cell constraints (#109)
  [UI] Use UIColor.STN extension (#108)
  Implement donations controller (#107)

# Conflicts:
#	SayTheirNames/Source/Controller/Home/HomeController.swift
#	SayTheirNames/Source/Controller/Home/HomeView.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review me This PR needs more reviewers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants