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

feat: add animation field to markers #852

Closed
wants to merge 1 commit into from
Closed

feat: add animation field to markers #852

wants to merge 1 commit into from

Conversation

xdimension
Copy link
Contributor

Feature to set animation for map markers

issue: Add animation field to markers #580

@GitCop
Copy link

GitCop commented Jan 13, 2017

There were the following issues with your Pull Request

  • Commit: 359af58
  • Commits must be in the following format: %{type}(%{scope}): %{description}

Contribution guidelines are available at https://github.com/SebastianM/angular2-google-maps/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

sebholstein
sebholstein requested changes Jan 14, 2017
View reviewed changes
Copy link
Owner

@sebholstein sebholstein left a comment

Choose a reason for hiding this comment

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

Thank you! Added some comments

src/core/directives/google-map-marker.ts Outdated
* Which animation to play when marker is added to a map.
* This can be "bounce" or "drop"
*/
animation: string;
Copy link
Owner

Choose a reason for hiding this comment

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

Should be animation: "BOUNCE" | "DROP" | null;

src/core/services/google-maps-types.ts
@@ -31,6 +31,7 @@ export interface Marker extends MVCObject {
setOpacity(opacity: number): void;
setVisible(visible: boolean): void;
setZIndex(zIndex: number): void;
setAnimation(animation: any): void;
Copy link
Owner

Choose a reason for hiding this comment

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

LGTM, this way, users can also provide a real google.maps.Animation

src/core/services/managers/marker-manager.ts Outdated
@@ -61,6 +63,12 @@ export class MarkerManager {
return this._markers.get(marker).then((m: Marker) => m.setZIndex(marker.zIndex));
}

updateAnimation(marker: SebmGoogleMapMarker): Promise<void> {
return this._markers.get(marker).then((m: Marker) =>
m.setAnimation(google.maps.Animation[marker.animation])
Copy link
Owner

Choose a reason for hiding this comment

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

this can also be null/undefined:

if (typeof marker.animation === 'string) {
  m.setAnimation(google.maps.Animation[marker.animation]);
} else {
  m.setAnimation(marker.animation);
}

src/core/services/managers/marker-manager.ts Outdated
@@ -73,6 +81,10 @@ export class MarkerManager {
title: marker.title
Copy link
Owner

Choose a reason for hiding this comment

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

please handle the animation here, please handle also null/undefined cases here as above.

src/core/services/managers/marker-manager.ts Outdated
@@ -73,6 +81,10 @@ export class MarkerManager {
title: marker.title
});
this._markers.set(marker, markerPromise);

markerPromise.then((m: Marker) =>
Copy link
Owner

Choose a reason for hiding this comment

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

see comment above

xdimension pushed a commit to xdimension/angular2-google-maps that referenced this pull request Jan 22, 2017
@iWebTouch
feat: add animation field to markers
f4f197c 
Feature to set animation for map markers
Updated according to comments in sebholstein#852

issue: Add animation field to markers sebholstein#580
@GitCop
Copy link

GitCop commented Jan 22, 2017

There were the following issues with your Pull Request

  • Commit: 359af58

  • Commits must be in the following format: %{type}(%{scope}): %{description}

  • Commit: f4f197c

  • Commits must be in the following format: %{type}(%{scope}): %{description}

Contribution guidelines are available at https://github.com/SebastianM/angular2-google-maps/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

@xdimension
Copy link
Contributor Author

Sorry for long update, please let me know if there's anything else should be fixed. Thank you.

@rhutchison
Copy link

@xdimension can you please reword the commit messages and squash so this can get merged. Looking forward to it -- thanks

xdimension pushed a commit to xdimension/angular2-google-maps that referenced this pull request Jan 29, 2017
@iWebTouch
feat: add animation field to markers
640e535 
Feature to set animation for map markers

issue: Add animation field to markers sebholstein#580

feat: add animation field to markers

Feature to set animation for map markers
Updated according to comments in sebholstein#852

issue: Add animation field to markers sebholstein#580
@GitCop
Copy link

GitCop commented Jan 29, 2017

There were the following issues with your Pull Request

  • Commit: 640e535
  • Commits must be in the following format: %{type}(%{scope}): %{description}

Contribution guidelines are available at https://github.com/SebastianM/angular2-google-maps/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

@xdimension xdimension force-pushed the feat-animation branch 2 times, most recently from 45a550d to b338c31 Compare January 29, 2017 06:39
@lazarljubenovic lazarljubenovic mentioned this pull request Feb 4, 2017
Closed
@rhutchison
Copy link

@xdimension thanks for fixing the PR - can you rebase?

@SebastianM can you please get this merged in once he fixes the PR?

Thanks guys

@xdimension
Copy link
Contributor Author

xdimension commented Feb 11, 2017 •
edited
Loading

@rhutchison My latest update is as below, I have done the squash. Not sure what I can do next, just let me know if I can help further. Thanks.

@rhutchison
Copy link

@xdimension it needs to be rebased on top of the latest changes - merge conflicts.

@sebholstein
Copy link
Owner

Can you fix the unit tests? Then I can merge it.

@iWebTouch
feat(AgmMarker): add animation field to markers
d99e292 
Feature to set animation for map markers

issue: Add animation field to markers sebholstein#580
@xdimension
Copy link
Contributor Author

@SebastianM Updated. I'm not really familiar with testing yet, so please review. Thanks.

@rbarcelos
Copy link

ETA of when will this be available?

@seanfar
Copy link

seanfar commented Jun 22, 2017

@xdimension thanks for your work getting this updated.
@SebastianM any possibility of getting this merged in before next update? Am I mistaken or is it not waiting on anything else? Looking forward to having this feature!

@mtszpater
Copy link

What's the status of this?

@xavier-experion
Copy link

Any update on this PR ?

@Freshchris01
Copy link

@SebastianM @xdimension Will this feature be merged soon?

@orijel
Copy link

orijel commented Dec 18, 2017

any updates on this?

@wtubog
Copy link

wtubog commented Jan 21, 2018

Hi, is this feature supported now?

@aepgit
Copy link

aepgit commented Feb 8, 2018

I'm assuming this will be in the next release? Any info on the schedule?

@miyelo
Copy link

miyelo commented Feb 23, 2018

Please please, or just let us access the native google map to add the marker following the google documentation :3
Awesome job

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebholstein sebholstein sebholstein requested changes

Assignees

@xdimension xdimension

Labels
comp: AgmMarker type: feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.