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 MarkerIcon type #1836

Merged
7 commits merged into from
Jul 16, 2020
Merged

Add MarkerIcon type #1836

7 commits merged into from
Jul 16, 2020

Conversation

darron1217
Copy link
Contributor

to fix #1622

@darron1217
Copy link
Contributor Author

I suggest changing iconUrl into icon, but I didn't applied it on my PR. (Cuz it's breaking change)

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks, here are a few improvements

packages/core/services/google-maps-types.ts Show resolved Hide resolved
packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
packages/core/services/google-maps-types.ts Outdated Show resolved Hide resolved
@@ -82,6 +82,15 @@ export interface MarkerLabel {
text: string;
}

export interface MarkerIcon {
anchor?: any | Point;
Copy link

Choose a reason for hiding this comment

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

why are all of these typed any | Something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because interface props without ? causes compile error.
For example, Size interface checks every prop exists even if I just want to use width & height

export interface Size {
    height: number;
    width: number;
    equals(other: Size): boolean;
    toString(): string;
}

I wanted to add question mark on every props of Size, but I just picked other solution.

Copy link

Choose a reason for hiding this comment

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

because Size is a class, not a literal interface. Google Maps API doesn't just support {width: 100, height: 200} . You're supposed to use their provided class with constructor. https://developers.google.com/maps/documentation/javascript/reference/coordinates#Size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I was using it in a wrong way

@ghost
Copy link

ghost commented Jul 16, 2020

Marker#setIcon should also allow Icon and GoogleSymbol

@darron1217
Copy link
Contributor Author

You've got a keen eye :)

@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #1836 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1836   +/-   ##
=======================================
  Coverage   47.97%   47.97%           
=======================================
  Files          47       47           
  Lines        1999     1999           
  Branches      177      177           
=======================================
  Hits          959      959           
  Misses       1036     1036           
  Partials        4        4           
Impacted Files Coverage Δ
packages/core/map-types.ts 0.00% <ø> (ø)
packages/core/services/google-maps-types.ts 100.00% <ø> (ø)
packages/core/directives/marker.ts 37.06% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ef77c...0d42ab0. Read the comment docs.

@ghost ghost assigned sebholstein Jul 16, 2020
@ghost ghost merged commit 4e38075 into sebholstein:master Jul 16, 2020
This pull request was closed.
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.

Need property icon rotation in agm-marker
2 participants