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

Revised pin style #46

Merged
merged 7 commits into from
Dec 20, 2015
Merged

Revised pin style #46

merged 7 commits into from
Dec 20, 2015

Conversation

neilang
Copy link
Owner

@neilang neilang commented Dec 19, 2015

Switching to an original set of push pins graphics for NAPinAnnotation. The old ones were out of date anyway.

Tests have been updated too.

These were originally included to provide UI consistency with the native counterpart MapKit. Although there were good intentions to present a consistent experience for the end user, I'm concerned this could be considered a violation of copyright so they need to be replaced.
Like the pins these need to be replaced. The title height was adjusted as it appeared off center with the new design.
Also removed note on using old version as this is no longer supported.
@dblock
Copy link
Collaborator

dblock commented Dec 20, 2015

Check the build, it should succeed on Travis. Also update CHANGELOG.

@neilang
Copy link
Owner Author

neilang commented Dec 20, 2015

@dblock I'm unsure what is causing the build to fail, do you have any ideas? Tests pass locally.

@tonyarnold
Copy link
Collaborator

It's CocoaPods. 🎉 😦

@tonyarnold tonyarnold closed this Dec 20, 2015
@tonyarnold tonyarnold reopened this Dec 20, 2015
@tonyarnold
Copy link
Collaborator

Now the tests are failing because there are tests that are doing image-based comparisons of the output 😕

I have… feelings… about how useful tests of this nature are in small demo projects (or any projects). Personally, I'd pull them out and move on to more important tasks.

This is not ideal, but I'm not sure how to fix the issues with the build server.
@neilang
Copy link
Owner Author

neilang commented Dec 20, 2015

Thanks @tonyarnold. I think you're right and unfortunately I do not know enough about how the test setup works to fix this myself right now. I'll temporarily remove them for now.

@orta
Copy link
Collaborator

orta commented Dec 20, 2015

At a guess, it was probably because the tests are recorded on a 64 bit sim, and the tests are being ran on another device. But, YOLO.

orta added a commit that referenced this pull request Dec 20, 2015
@orta orta merged commit 9316136 into master Dec 20, 2015
@dblock
Copy link
Collaborator

dblock commented Dec 20, 2015

FWIW there was a lot of effort put into making snapshot tests work in this project and they catch real bugs, otherwise there's literally no way to know whether map annotations work at all. I opened #47.

orta added a commit that referenced this pull request Dec 20, 2015
orta added a commit that referenced this pull request Dec 20, 2015
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.

4 participants