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

Add new CardPartMapView #185

Merged
merged 12 commits into from Oct 10, 2019
Merged

Add new CardPartMapView #185

merged 12 commits into from Oct 10, 2019

Conversation

mvdmakesthings
Copy link
Contributor

Related to #183

Alright! Putting myself out there for the first time. Here's my first stab at it. I added three different properties that can be accessed via the RX binding syntax:

  • location
  • map type
  • zoom level (span)

I also made sure the the underlying MKMapView was available so that users could access its delegate functions or interact with the map directly as needed.

In my example, I showed how you could bind directly to a CardPartTextField's value and change the location of the map. At the same time, I'm cycling through different "zoom levels".

I'm totally open to adding more bindable properties if you think it needs more.


On a personal note, this was a lot of fun and this library is very well organized. Well done to all of the other contributors that have worked on this project.

This also includes some cleanup of the zoom property, as it isn't being used actively in the example.
Initially, I chose to use the mapview setRegion signature that allowed latitudinal and longitudinal meter properties to define the "zoom" level of the map. After reading online, it seemed like the more appropriate way to do this is to use the span (MKCoordinateSpan) property to set the map zoom level.
@badrinathvm
Copy link
Contributor

badrinathvm commented Oct 7, 2019

Below code can be simplified even more cleaner

public override func updateConstraints() {
    mapView.layout {
         $0.top == self.topAnchor
         $0.leading == self.leadingAnchor
         $0.trailing == self.trailingAnchor
         $0.bottom == self.bottomAnchor
      }
        super.updateConstraints()
    }

This layout{} method also sets translatesAutoResizing to false as well..

$0.leading == self.leadingAnchor
$0.trailing == self.trailingAnchor
$0.bottom == self.bottomAnchor
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome , looks very clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was very nice cleanup. Thanks for that.

@croossin
Copy link
Contributor

croossin commented Oct 7, 2019

Awesome work, @codethebeard!

I think a few potentially awesome properties to expose as bindable would be:

  • Annotations
  • userLocation

It's awesome that you made the mapView public so that the developer can easily access it whenever needed.

Again, we appreciate the contributions and are open to hear about any other features you'd like to see in CardParts. Thanks!

@mvdmakesthings
Copy link
Contributor Author

@croossin Thanks man! I appreciate the opportunity. I can certainly add those properties.

@mvdmakesthings
Copy link
Contributor Author

@croossin

I think a few potentially awesome properties to expose as bindable would be:

  • Annotations
  • userLocation

So I just started looking at this and realized that both the MKMapView.annotations [MKAnnotation] and userLocation MKUserLocation properties are get-only properties and probably wouldn't work as bindable properties.

I could potentially expose them as a ControlProperty possibly which might be able to give you some mechanism to observe on those changes, however in the case of userLocation the recommended way to obtain that value is to implement the MKMapView's mapView(_: MKMapView, didUpdate: MKUserLocation) delegate method which would limit the developers ability to safely implement the MKMapView delegate themselves. (basically overriding the delegate)

Any thoughts? I'm a little new to this level of RX but would love to learn a thing if there's a way to properly do what you're asking.

@croossin
Copy link
Contributor

croossin commented Oct 8, 2019

@codethebeard What if for the annotations we used addAnnotations([MKAnnotation]) and removeAnnotations([MKAnnotation])? My only concern would be performance.

Maybe we take a look at how RxMKMapView deals with annotation binding.

I just felt that binding annotations to the mapview would be a key request, but we can discuss if we want to have that as a "to-do" feature.

@mvdmakesthings
Copy link
Contributor Author

mvdmakesthings commented Oct 8, 2019

@croossin Yeah, I can see a lot of people wanting that type of feature. I'll start taking a look at the RxMKMapView when I get some free time today to see what they are doing and report back.

@croossin
Copy link
Contributor

croossin commented Oct 8, 2019

@codethebeard Awesome! I'm sure we can figure out this problem 🤓

@croossin croossin added enhancement New feature or request hacktoberfest A label dedicated to issues that would be participating in Hacktoberferst labels Oct 8, 2019
@mvdmakesthings
Copy link
Contributor Author

@croossin So, I took a look at RxMKMapView to see how they are doing the annotation binding. I gave it the ol' college try but that rabbit hole goes pretty deep and it's honestly a little over my head. Maybe someone out there in the community with more Swift/RxSwift experience and time could be able to make this Card Part what it really needs to be.

@croossin
Copy link
Contributor

No worries at all @codethebeard. Awesome work thus far. I will go ahead and merge/release this tomorrow! We can add that support later on.

Feel free to email (chase_roossin@intuit.com) your t-shirt size as well as mailing address if you’d like some Intuit swag for helping participate and contribute during Hacktoberfest! We really appreciate all the work.

@croossin croossin merged commit bdb7f34 into intuit:master Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest A label dedicated to issues that would be participating in Hacktoberferst
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants