feat: Add placeId to map click events #1606

yosigolan · 2019-02-24T11:52:01Z

feat: Add placeId to map click events

When clicking on a point of interest on a map the emitting event will include the placeId of the location clicked.

closes issue #691.

allow disabling a "point of interest" default info window. use it by passing a boolean value to the new property isToShowDefaultInfoWindowForIcons when initializing the map. default value is true. closes issue sebholstein#1539.

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691.

codecov · 2019-02-24T11:54:41Z

Codecov Report

Merging #1606 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   33.17%   33.11%   -0.07%     
==========================================
  Files          37       37              
  Lines        1640     1643       +3     
  Branches      128      129       +1     
==========================================
  Hits          544      544              
- Misses       1094     1097       +3     
  Partials        2        2

Impacted Files	Coverage Δ
packages/core/directives/map.ts	`0% <0%> (ø)`	⬆️

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 49890c5...d6d5a6a. Read the comment docs.

packages/core/directives/map.ts

benjcallaghan

The general implementation of the new feature looks good. In its current form, it is suitable for use in my own projects. Most of your changes appear to be style changes, some of which go against the Google JavaScript Style Guide. I recommend that you run the provided npm run clang:format script and commit the resulting changes.

The two automated checks that didn't pass are both related to unit testing. The Contributing Guidelines specify that appropriate test cases should be added to any new feature. I recommend adding some unit tests to verify the new functionality that you have added.

Again, the functionality looks great. All that is left is housekeeping to improve the code quality.

benjcallaghan · 2019-03-06T01:28:37Z

packages/core/directives/map.ts

+   * When this property is set to npm run buildfalse, the info window will not be shown but the click event
+   * will still fire
+   */
+  @Input() isToShowDefaultInfoWindowForIcons: boolean = true;


The input name is rather verbose. A shorter name, such as showDefaultInfoWindow, would be easier to understand and would avoid cluttering user code down the line.

@benjcallaghan i have committed fixes to all of your comments.
Thanks

packages/core/directives/map.ts

benjcallaghan · 2019-03-06T01:31:48Z

packages/core/map-types.ts

-export interface MouseEvent { coords: LatLngLiteral; }
+export interface MouseEvent {
+  coords: LatLngLiteral;
+  placeId: string;


Because some MouseEvent objects do not have a placeId, such as when the user clicks on a blank portion of the map, the property on the interface should be optional.

Suggested change

placeId: string;

placeId?: string;

benjcallaghan · 2019-03-06T01:35:08Z

packages/core/directives/map.ts

+              lat: event.latLng.lat(),
+              lng: event.latLng.lng()
+            },
+            placeId: null


The value for placeId can be in-lined here without a conditional. If the Google event doesn't have that property, then it is set to undefined, which follows the existing pattern by Google and matches the previous behavior.

Suggested change

placeId: null

placeId: (<google.maps.IconMouseEvent>event).placeId

benjcallaghan · 2019-03-06T01:36:45Z

packages/core/directives/map.ts

+            placeId: null
+          };
+          // we cant use instanceOf since the IconMouseEvent is an interface
+          if ((<any>event).placeId !== undefined && (<any>event).placeId !== null) {


The two conditions here can be simplified since both undefined and null are "falsy" values in JavaScript.

Suggested change

if ((<any>event).placeId !== undefined && (<any>event).placeId !== null) {

if (value.placeId && !this.isToShowDefaultInfoWindowForIcons) {

@benjcallaghan

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691. fixes for @benjcallaghan code review comments

benjcallaghan · 2019-03-30T18:13:26Z

@SebastianM What further changes do we need to make, if any, to get this approved for production use?

ghost

In addition to the marked changes, please remove all the whitespace, formatting changes, since they are outside the scope of this PR. If you want, open a separate PR with all the styling chores.

Thanks for your contribution!

package.json

packages/core/directives/map.ts

@doom777

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691. fixes for @doom777 code review comments

yosigolan · 2019-06-02T09:48:44Z

Hey @doom777
fixed all your comments.

ghost

Hey, @yosigolan thanks so much for putting in work.

Please correct the few minor whitespace changes I pointed out, and I'd be happy to test the PR out and merge into the code.

Thanks in advance!

packages/core/directives/map.ts

@doom777

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691. fixes for @doom777 code review comments

yosigolan · 2019-06-02T11:16:56Z

@doom777
Done

yosigolan added 2 commits February 24, 2019 13:43

feat: Add placeId to map click events

37b694c

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691.

ghost reviewed Feb 24, 2019

View reviewed changes

packages/core/directives/map.ts Outdated Show resolved Hide resolved

benjcallaghan reviewed Mar 6, 2019

View reviewed changes

feat: Add placeId to map click events

34ec1ea

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691. fixes for @benjcallaghan code review comments

walkingriver mentioned this pull request Mar 6, 2019

place_id is missing in click event #1614

Closed

ghost suggested changes May 30, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

packages/core/directives/map.ts Outdated Show resolved Hide resolved

packages/core/directives/map.ts Outdated Show resolved Hide resolved

ghost added the needs: changes label May 30, 2019

feat: Add placeId to map click events

e057efc

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691. fixes for @doom777 code review comments

Merge branch 'master' into add-placeid

e7fe738

ghost suggested changes Jun 2, 2019

View reviewed changes

packages/core/directives/map.ts Outdated Show resolved Hide resolved

packages/core/directives/map.ts Outdated Show resolved Hide resolved

packages/core/directives/map.ts Outdated Show resolved Hide resolved

packages/core/directives/map.ts Outdated Show resolved Hide resolved

yosigolan added 2 commits June 2, 2019 14:15

feat: Add placeId to map click events

813736e

When clicking on a point of interest on a map the emiting event will include the placeId of the location clicked. closes issue sebholstein#691. fixes for @doom777 code review comments

Merge remote-tracking branch 'origin/add-placeid' into add-placeid

5a14745

ghost added needs: review and removed needs: changes labels Jun 2, 2019

ghost self-assigned this Jun 2, 2019

ghost approved these changes Jun 2, 2019

View reviewed changes

Merge branch 'master' into add-placeid

d6d5a6a

ghost merged commit 34f651b into sebholstein:master Jun 3, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add placeId to map click events #1606

feat: Add placeId to map click events #1606

yosigolan commented Feb 24, 2019

codecov bot commented Feb 24, 2019 •

edited

Loading

benjcallaghan left a comment

benjcallaghan Mar 6, 2019

yosigolan Mar 6, 2019

benjcallaghan Mar 6, 2019

benjcallaghan Mar 6, 2019

benjcallaghan Mar 6, 2019

benjcallaghan commented Mar 30, 2019

ghost left a comment

yosigolan commented Jun 2, 2019

ghost left a comment

yosigolan commented Jun 2, 2019

	placeId: null
	placeId: (<google.maps.IconMouseEvent>event).placeId

	if ((<any>event).placeId !== undefined && (<any>event).placeId !== null) {
	if (value.placeId && !this.isToShowDefaultInfoWindowForIcons) {

feat: Add placeId to map click events #1606

feat: Add placeId to map click events #1606

Conversation

yosigolan commented Feb 24, 2019

codecov bot commented Feb 24, 2019 • edited Loading

Codecov Report

benjcallaghan left a comment

Choose a reason for hiding this comment

benjcallaghan Mar 6, 2019

Choose a reason for hiding this comment

yosigolan Mar 6, 2019

Choose a reason for hiding this comment

benjcallaghan Mar 6, 2019

Choose a reason for hiding this comment

benjcallaghan Mar 6, 2019

Choose a reason for hiding this comment

benjcallaghan Mar 6, 2019

Choose a reason for hiding this comment

benjcallaghan commented Mar 30, 2019

ghost left a comment

Choose a reason for hiding this comment

yosigolan commented Jun 2, 2019

ghost left a comment

Choose a reason for hiding this comment

yosigolan commented Jun 2, 2019

codecov bot commented Feb 24, 2019 •

edited

Loading