Skip to content
This repository has been archived by the owner on Jun 2, 2019. It is now read-only.

fixed tinting issues #561

Closed
wants to merge 19 commits into from
Closed

Conversation

MillerApps
Copy link
Contributor

@MillerApps MillerApps commented Apr 2, 2018

I am not a fan of this solution. Feel free to make any changes that you think would improve this "hack"!

Here is what it looks like in the app:
image

@MillerApps MillerApps requested a review from vikmeup April 2, 2018 02:27
@trust-bot
Copy link

trust-bot commented Apr 2, 2018

SwiftLint found issues

Warnings

File Line Reason
BrowserViewController.swift 188 Prefer the new block based KVO API with keypaths when using Swift 3.2 or later.
SettingsViewController.swift 53 Function body should span 80 lines or less excluding comments and whitespace: currently spans 121 lines

Generated by 🚫 Danger

@@ -51,6 +51,12 @@ class RequestCoordinator: Coordinator {
applicationActivities: nil
)
activityViewController.popoverPresentationController?.barButtonItem = sender
activityViewController.setActivityCompletion {
Copy link
Contributor

Choose a reason for hiding this comment

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

did you consider reusing this functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it didn't work as intended.

@vikmeup
Copy link
Contributor

vikmeup commented Apr 2, 2018

@MillerApps does it have any side effects?

@MillerApps
Copy link
Contributor Author

MillerApps commented Apr 2, 2018

@vikmeup as far as I can tell no.

@vikmeup
Copy link
Contributor

vikmeup commented Apr 2, 2018

Let's create ActivityViewController that implements that functionality

@MillerApps
Copy link
Contributor Author

Okay, I'll do it tomorrow.

@MillerApps
Copy link
Contributor Author

I won't have time to do this today.

@vikmeup
Copy link
Contributor

vikmeup commented Apr 2, 2018

@MillerApps That’s like 2 minute fix. I can take on this

@MillerApps
Copy link
Contributor Author

@vikmeup Okay, sounds good!

Copy link
Contributor

@vikmeup vikmeup left a comment

Choose a reason for hiding this comment

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

@MillerApps can you make this since it's on your fork.

you are part of this repo, you should be able to work on repo branches.

extension UIActivityViewController {
func setActivityCompletion(completion: @escaping (() -> Void)) {
self.completionWithItemsHandler = { _, completed, _, _ in
if completed || !completed {
Copy link
Contributor

Choose a reason for hiding this comment

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

completed || !completed this covers all cases, is this still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, didn't seem to affect anything.

@@ -249,6 +249,12 @@ class BrowserViewController: UIViewController {
let controller = makeShareController(url: url)
controller.popoverPresentationController?.sourceView = navigationController.view
controller.popoverPresentationController?.sourceRect = navigationController.view.centerRect
controller.setActivityCompletion {
self.navigationController?.navigationBar.barTintColor = Colors.darkBlue
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add this colors to AppStyle ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MillerApps
Copy link
Contributor Author

@vikmeup I’ll get it done today when I get a chance.

Sent with GitHawk

@MillerApps MillerApps closed this Apr 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants