Add map attribution #181

sarahsnow1 · 2017-02-24T23:39:14Z

Adds attribution button to bottom left of map which links to https://mapzen.com/rights/ when clicked
Fixes map's view height to consider tab bar
Considers the new mapView property to be the main view and adds all other subview to it instead of to the view controller's view
Fixes find me button positioning
Adds string resource file
Adds dimension resource file

Closes #87

msmollin · 2017-02-28T16:57:51Z

Dimens.swift

+
+import UIKit
+
+open class Dimens {


Should this be open or public visibility ? Do you expect people to be subclassing this?

Not that it matters a ton, but you can import Foundation instead of UIKit since you're not using anything UIKit specific.

Not a fan of name abbreviations - lets call this Dimensions or something else.

Can you document the class?

Public would make more sense...they probably wont subclass it, just access the constants

Updated

Renamed to Dimensions

Documented

msmollin · 2017-02-28T17:00:08Z

src/MapViewController.swift


+  var application : ApplicationProtocol = UIApplication.shared


Probably could be a let constant.

msmollin · 2017-02-28T17:09:08Z

src/MapViewController.swift

@@ -446,6 +451,9 @@ open class MapViewController: UIViewController, LocationManagerDelegate {
    findMeButton.setBackgroundImage(UIImage(named: "ic_find_me_pressed"), for: [.selected])
    findMeButton.backgroundColor = UIColor.white
    findMeButton.autoresizingMask = [.flexibleTopMargin, .flexibleLeftMargin]
+    let viewRect = mapView.bounds


This won't work correctly. The createFindMeButton() function is called in the initializer of the VC. Referencing the view at this stage in the lifecycle will force view generation to happen outside of the proper lifecycle events.

What are you attempting to solve by moving this around?

I was focused on removing it from viewWillAppear since it doesnt need to be added every time the view appears but youre right, this isn't the place to add it. I've moved calling of this method from the initializer into viewDidLoad

msmollin · 2017-02-28T17:18:22Z

src/MapViewController.swift

+  //MARK: - private
+
+  private func setupTgControllerView() {
+    tgViewController.view.frame = CGRect(x: 0, y: 0, width: self.view.bounds.width, height: self.view.bounds.height-tabBarHeight)


Couple issues:

The reason you're needing to subtract the height here is because you're not using constraints. The view should adopt the size of its superview. So what I would do here is try here is to use the Autresizing Mask to keep it simple. Something like .autoresizingMask = [.flexibleTopMargin, .flexibleLeftMargin, .flexibleRightMargin, .flexibleBottomMargin] should do what you want.

The next issue is self.view doesn't always have a bounds.width in viewDidLoad, which is when this is being called. This is because of constraints. You really only get the right size of that once viewDidLayoutSubviews is finished. So, if you swap to using constraints, you shouldn't have to set the frame.

The view's height doesn't adjust for the tab bar so I do need to adjust for it. I've updated setting of the tgViewController to use constraints.

msmollin · 2017-02-28T17:22:45Z

src/MapViewController.swift

+    attributionBtn.titleLabel?.font = UIFont.systemFont(ofSize: 14)
+    attributionBtn.addTarget(self, action: #selector(openMapzenTerms), for: .touchUpInside)
+    attributionBtn.sizeToFit()
+    attributionBtn.translatesAutoresizingMaskIntoConstraints = false


This should automatically get set to false when you add new constraints.

Thats when constraints are set in IB:

If you want to use Auto Layout to dynamically calculate the size and position of your view, you must set this property to false, and then provide a non ambiguous, nonconflicting set of constraints for the view. By default, the property is set to true for any view you programmatically create. If you add views in Interface Builder, the system automatically sets this property to false.

Got it. 👍

msmollin · 2017-02-28T17:32:21Z

Localizable.strings

@@ -0,0 +1,9 @@
+/* 
+  Localizable.strings


Did you check to make sure this gets imported when pulled in via cocoapods?

No I didnt, will do that now

Remove the strings file and will properly handle this in a follow up PR. Ticket here: #187

msmollin · 2017-03-01T22:07:33Z

ios-sdkTests/MapViewControllerTests.swift

@@ -224,6 +226,7 @@ class MapViewControllerTests: XCTestCase {
  }

  func testFindMeButtonInitialState() {
+    controller.viewDidLoad()


Mmmmm we should open the scope of setupFindMeButton() to Internal so we can test it. That'll still accomplish keeping it private from implementing devs.

msmollin · 2017-03-01T22:09:17Z

ios-sdk.xcodeproj/project.pbxproj

 			isa = PBXGroup;
 			children = (
-				7D7DAE191E60B00400FFCA6F /* dimens */,
-				7D7DAE181E60AFFE00FFCA6F /* strings */,
+				7D7847241E6775EA000D56CA /* Dimensions.swift */,


Did this get moved into src on disk?

Yes. Its now in src/dimensions/Dimensions.swift

I'm thinking we might want to move our controllers, managers, extensions into separate folders in later PR too

sarahsnow1 added the in progress label Feb 24, 2017

sarahsnow1 requested review from ecgreb, msmollin and karimnaaji February 24, 2017 23:42

sarahsnow1 assigned msmollin and karimnaaji Feb 24, 2017

msmollin requested changes Feb 28, 2017

View reviewed changes

sarahsnow1 force-pushed the 87-attribution branch 3 times, most recently from e42072d to 1afc55c Compare March 1, 2017 21:36

msmollin requested changes Mar 1, 2017

View reviewed changes

Sarah Lensing added 6 commits March 1, 2017 17:27

Add attribution button

863eeff

Fix find me button positioning

7b8239f

Add attribution to mapView for consistency

10991e4

Add attribution test

7e0549c

Update Dimensions file

852d01a

Update map subview constraints

18b277c

msmollin approved these changes Mar 1, 2017

View reviewed changes

Sarah Lensing added 2 commits March 1, 2017 17:33

Remove strings file for now

8c92df6

Change setup method scope to internal

6ee219f

sarahsnow1 force-pushed the 87-attribution branch from a4c47f2 to 6ee219f Compare March 1, 2017 22:34

msmollin merged commit 8fb18a9 into master Mar 1, 2017

msmollin deleted the 87-attribution branch March 1, 2017 22:41

msmollin removed the in progress label Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add map attribution #181

Add map attribution #181

sarahsnow1 commented Feb 24, 2017 •

edited

Loading

msmollin Feb 28, 2017

sarahsnow1 Feb 28, 2017

msmollin Feb 28, 2017

sarahsnow1 Feb 28, 2017

msmollin Feb 28, 2017

sarahsnow1 Feb 28, 2017

msmollin Feb 28, 2017

sarahsnow1 Feb 28, 2017

msmollin Feb 28, 2017

sarahsnow1 Feb 28, 2017

msmollin Mar 1, 2017

msmollin Feb 28, 2017

sarahsnow1 Mar 1, 2017

sarahsnow1 Mar 1, 2017

msmollin Mar 1, 2017

sarahsnow1 Mar 1, 2017

msmollin Mar 1, 2017

sarahsnow1 Mar 1, 2017


		var application : ApplicationProtocol = UIApplication.shared

Add map attribution #181

Add map attribution #181

Conversation

sarahsnow1 commented Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahsnow1 commented Feb 24, 2017 •

edited

Loading