-
Notifications
You must be signed in to change notification settings - Fork 821
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
Auto-convert string to Number for marker input #1424
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1424 +/- ##
==========================================
- Coverage 28.45% 28.37% -0.09%
==========================================
Files 30 30
Lines 1353 1357 +4
Branches 182 184 +2
==========================================
Hits 385 385
- Misses 966 970 +4
Partials 2 2
Continue to review full report at Codecov.
|
packages/core/directives/marker.ts
Outdated
@@ -158,8 +158,11 @@ export class AgmMarker implements OnDestroy, OnChanges, AfterContentInit { | |||
|
|||
/** @internal */ | |||
ngOnChanges(changes: {[key: string]: SimpleChange}) { | |||
if (typeof this.latitude !== 'number' || typeof this.longitude !== 'number') { | |||
return; | |||
if (typeof this.latitude === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks the case that the marker only gets added to the manager if latitude/longitude are numbers. Can you add these check underneath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad! I added the check again
Is this still relevant? If so I'd appreciate a new review |
I stumbled upon this PR, which seems to be put on hold by the original author.
I thought I'd give it a try so I incorporated your change request. Let me know if this is different from what you had in mind.
Fixes #771