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

Enhancements #68

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Enhancements #68

wants to merge 8 commits into from

Conversation

Yoloabdo
Copy link
Contributor

A few updates for code syntax:

moving uicolor functions extension to vars for more swifty way
rewriting navigate to function in settings to be generic with less lines

this was done as a part of our community activity (reading code club) in #swiftCairo

thanks

@BasThomas
Copy link
Collaborator

Hey @Yoloabdo, thanks! This does not look entirely mergeable though — especially the “theirs” and “ours” and “base” files.

Sent with GitHawk

@Yoloabdo
Copy link
Contributor Author

I just saw that, weird, I'll try to fix it

remove unwanted files
@Yoloabdo
Copy link
Contributor Author

it's fixed now, removed those files

@@ -36,15 +36,15 @@ private extension AppDelegate {

func configureStyling() {

window?.tintColor = UIColor.trySwiftNavigationBarColor()
window?.tintColor = UIColor.trySwiftNavigationBarColor
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can omit the UIColor prefixes as these are inferred.

@@ -124,24 +142,27 @@ extension MoreTableViewController {
return cell
}


Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this for?

acknowledgementesViewController.headerText = "We ❤️ Open Source Software".localized()

performSegue(withIdentifier: moreDetailSegue, sender: acknowledgementesViewController)
func navigateTo<T: UIViewController>(id: String = "moreDetailSegue", _ controller: () -> T){
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔, I don't really like this signature. Especially when used without spacing for the {}'s. What about func navigate<T: UIViewController>(to controller: () -> T, id: String = "moreDetailSegue")? Then use it with the parameter:

navigate(to: { acknowledgement })

Thoughts, @NatashaTheRobot?

Copy link
Contributor

Choose a reason for hiding this comment

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

@BasThomas agreed - your version looks better :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that I think of it... is there any reason why this couldn't just accept a UIViewController instead of a closure?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds much simpler that way!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks better indeed, I've replaced it

@@ -0,0 +1,27 @@
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an example to separate 3rd library from code so it's to be replaced easily anytime, I'll remove it

@@ -14,6 +14,7 @@ extension UITableView {

let nib = UINib(nibName: T.nibName, bundle: nil)
register(nib, forCellReuseIdentifier: T.reuseIdentifier)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can omit?

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