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

Depend on Polyline.framework #53

Merged
merged 5 commits into from Nov 22, 2017
Merged

Depend on Polyline.framework #53

merged 5 commits into from Nov 22, 2017

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Feb 23, 2017

Added a dependency on raphaelmor/Polyline@f86f34d. (We can pin to a newer version or release once raphaelmor/Polyline#40 lands.)

Fixes #11.

/cc @incanus

@1ec5 1ec5 self-assigned this Feb 23, 2017
@1ec5 1ec5 requested a review from tmcw February 23, 2017 09:49
@1ec5 1ec5 mentioned this pull request Mar 14, 2017
Closed
@1ec5 1ec5 added the build label Mar 14, 2017
1ec5
1ec5 commented Mar 25, 2017
View reviewed changes
MapboxStaticTests/MapboxStaticTests.swift Outdated
@@ -370,7 +370,7 @@ class MapboxStaticTests: XCTestCase {
let fillColor = Color.red
let fillColorRaw = "ff0000"
let fillOpacity = 0.25
let encodedPolyline = "(upztG%60jxkVn@al@bo@nFWzuAaTcAyZen@)"
let encodedPolyline = "(upztG%60jxkVn@al@bo@pFWzuAaTcAyZgn@)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmcw, should we be concerned that there were differences in how Polyline encoded the polyline versus our homegrown encoder’s version? In this case at least, there didn’t appear to be any visual difference.

@1ec5
Depend on Polyline.framework
c2fa4b5
@1ec5
Embed Polyline.framework in test bundles
9a51a00
@1ec5
Updated encoded polyline test fixture
45f66af 
Polyline.framework encodes the polyline slightly differently, but there doesn’t appear to be any discernible difference in the output image.
@1ec5
Embed MapboxStatic in example applications
d280f4f
@1ec5 1ec5 requested review from bsudekum and removed request for tmcw November 20, 2017 21:21
@1ec5
Copy link
Contributor Author

1ec5 commented Nov 20, 2017

@bsudekum, this PR will make it much easier to upgrade to Swift 4.

bsudekum
bsudekum reviewed Nov 20, 2017
View reviewed changes
Cartfile
@@ -0,0 +1,2 @@
github "raphaelmor/Polyline" ~> 4.2
Copy link

Choose a reason for hiding this comment

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

This version is explicitly for swift 4. I'm not sure this will pass unless this changed to v4.1.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just upgraded the Bitrise apps to use Xcode 9 instead of Xcode 8.3. The builds should pass now.

@1ec5
Copy link
Contributor Author

1ec5 commented Nov 21, 2017

This test currently fails:

MapboxStaticTests.ClassicOverlayTests
  testGeoJSONObject, XCTAssertNotNil failed - 
  /Users/vagrant/git/MapboxStaticTests/ClassicOverlayTests.swift:108
        
        XCTAssertNotNil(Snapshot(options: options, accessToken: BogusToken).image)
    }

@1ec5
Copy link
Contributor Author

1ec5 commented Nov 21, 2017

Upgrading to Xcode 9 means the macOS 10.13 SDK uses different RGB component values for all the NSColor constants than UIColor does. 🙄

@1ec5
Updated tests
179016c 
In the iOS 11 and macOS 10.13 SDKs, JSONSerialization adds an extra digit of precision. Also, in the macOS 10.13 SDK, the RGB components of NSColor’s class properties differ from those of UIColor’s similarly named class properties.
@1ec5
Copy link
Contributor Author

1ec5 commented Nov 21, 2017

Fixed the tests in 179016c.

In the iOS 11 and macOS 10.13 SDKs, JSONSerialization adds an extra digit of precision. Also, in the macOS 10.13 SDK, the RGB components of NSColor’s class properties differ from those of UIColor’s similarly named class properties.

@1ec5
Copy link
Contributor Author

1ec5 commented Nov 22, 2017

@bsudekum @frederoni @friedbunny, this PR is ready for review.

frederoni
frederoni reviewed Nov 22, 2017
View reviewed changes
MapboxStatic.xcodeproj/project.pbxproj
@@ -1071,6 +1125,10 @@
DYLIB_COMPATIBILITY_VERSION = 1;
DYLIB_CURRENT_VERSION = 7;
DYLIB_INSTALL_NAME_BASE = "@rpath";
FRAMEWORK_SEARCH_PATHS = (
"$(inherited)",
"$(PROJECT_DIR)/Carthage/Build/iOS",
Copy link
Contributor

@frederoni frederoni Nov 22, 2017 •
edited

Choose a reason for hiding this comment

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

This works but I think the recommended way is https://github.com/Carthage/Carthage#if-youre-building-for-ios-tvos-or-watchos
Nvm, I thought this was the example target.

@1ec5 1ec5 merged commit fe46792 into master Nov 22, 2017
@1ec5 1ec5 deleted the 1ec5-polyline-11 branch November 22, 2017 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsudekum bsudekum bsudekum left review comments

@frederoni frederoni frederoni approved these changes

@friedbunny friedbunny Awaiting requested review from friedbunny

Assignees

@1ec5 1ec5

Labels
build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@1ec5 @frederoni @bsudekum