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

mgl-map firing click twice #107

Closed
Arekusei opened this issue Feb 26, 2019 · 7 comments
Closed

mgl-map firing click twice #107

Arekusei opened this issue Feb 26, 2019 · 7 comments
Assignees
Labels

Comments

@Arekusei
Copy link

html:

<mgl-map [style]="'mapbox://styles/mapbox/streets-v9'"
         [zoom]="[5]"
         [center]="[10.61527, 51.270658]"
         [cursorStyle]="cursorStyle"
         (click)="mapClicked($event)"
></mgl-map>

listener:

  mapClicked($event: MapMouseEvent) {
    console.log('Global listener on the map "clicked"', $event);
  }

console:
angular_mapbox_bug

@magnolo
Copy link

magnolo commented Jun 19, 2019

hey... experienced the same behavior just right now. Does this seem to fire the angular internal (click) event and the ngx-map (click) event both? Maybe just a naming thing?

Thank u all for this module.
love M

@Wykks
Copy link
Owner

Wykks commented Jul 3, 2019

Yep that's probably a name conflict 😅
This is rarely used I believe. Will be fixed in V4

@Wykks Wykks mentioned this issue Jul 3, 2019
7 tasks
@dmytro-gokun
Copy link
Collaborator

@Wykks I've tried enabling rule "no-output-native" like you suggested here: #149

That shows that there are quite a lot of conflicts like this one. 21 to be exact:

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/control/geocoder-control.directive.ts:70:3
ERROR: 70:3   no-output-native  In the class "GeocoderControlDirective", the output property "result" should not be named or renamed as a native event
ERROR: 71:3   no-output-native  In the class "GeocoderControlDirective", the output property "error" should not be named or renamed as a native event 

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/draggable/draggable.directive.ts:29:3
ERROR: 29:3   no-output-native  In the class "DraggableDirective", the output property "drag" should not be named or renamed as a native event        

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/image/image.component.ts:30:3
ERROR: 30:3   no-output-native  In the class "ImageComponent", the output property "error" should not be named or renamed as a native event

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/layer/layer.component.ts:59:3
ERROR: 59:3   no-output-native  In the class "LayerComponent", the output property "click" should not be named or renamed as a native event

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/map/map.component.ts:97:3
ERROR: 97:3   no-output-native  In the class "MapComponent", the output property "resize" should not be named or renamed as a native event
ERROR: 102:3  no-output-native  In the class "MapComponent", the output property "click" should not be named or renamed as a native event
ERROR: 113:3  no-output-native  In the class "MapComponent", the output property "wheel" should not be named or renamed as a native event
ERROR: 118:3  no-output-native  In the class "MapComponent", the output property "drag" should not be named or renamed as a native event
ERROR: 134:3  no-output-native  In the class "MapComponent", the output property "load" should not be named or renamed as a native event
ERROR: 137:3  no-output-native  In the class "MapComponent", the output property "error" should not be named or renamed as a native event

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/marker/marker.component.ts:38:3
ERROR: 38:3   no-output-native  In the class "MarkerComponent", the output property "drag" should not be named or renamed as a native event

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/popup/popup.component.ts:37:3
ERROR: 97:3   no-output-native  In the class "MapComponent", the output property "resize" should not be named or renamed as a native event
ERROR: 102:3  no-output-native  In the class "MapComponent", the output property "click" should not be named or renamed as a native event
ERROR: 113:3  no-output-native  In the class "MapComponent", the output property "wheel" should not be named or renamed as a native event
ERROR: 118:3  no-output-native  In the class "MapComponent", the output property "drag" should not be named or renamed as a native event
ERROR: 134:3  no-output-native  In the class "MapComponent", the output property "load" should not be named or renamed as a native event
ERROR: 137:3  no-output-native  In the class "MapComponent", the output property "error" should not be named or renamed as a native event

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/marker/marker.component.ts:38:3
ERROR: 38:3   no-output-native  In the class "MarkerComponent", the output property "drag" should not be named or renamed as a native event

ngx-mapbox-gl/projects/ngx-mapbox-gl/src/lib/popup/popup.component.ts:37:3
ERROR: 37:3   no-output-native  In the class "PopupComponent", the output property "close" should not be named or renamed as a native event
ERROR: 38:3   no-output-native  In the class "PopupComponent", the output property "open" should not be named or renamed as a native event

I do not think that renaming all of these is a good idea. First of all, i'm not sure what is a good naming convention to use. Angular Style Guide directly warn against using "on" prefix: https://angular.io/guide/styleguide#style-05-16 At the same time, many 3rd party libs use just that convention :)

Secondly, if we rename all those 21 events... well it's a huge breaking change.
Thirdly, if we rename those affected and leave unaffected as they are, we may end up with half of events using one naming scheme and the other half using another one. That will confuse people a lot.

Do you have any thought on this subject?

@Wykks
Copy link
Owner

Wykks commented Aug 1, 2019

I didn't expect that 😅 .

It remind me that material did something similar a while ago (eg https://github.com/angular/components/pull/8053/files). Maybe for the same issues?

When there's a conflict with native output, we can just add context, like that: click => mapClick
I prefer that instead of onClick or clicked.
(Because it's weird to explain that click bind to the dom element, and clicked bind to the actual map 😄 ).

For the v4, we can just rename click event (also dblclick) because it's an actual issue. The may add the others later, while deprecating old output names.

Thirdly, if we rename those affected and leave unaffected as they are, we may end up with half of events using one naming scheme and the other half using another one. That will confuse people a lot.
If we add context, then I believe it's fine. Devs will be able to explicitly use native or mapbox events.
(Some are not really common like result / error but well... it's the standard...)

(note: sorry for the delay here, I disconnected a bit these days...)

@dmytro-gokun
Copy link
Collaborator

@Wykks I'm not sure if i like this "mapClick" convention. It's a bit too wordy + a lot of repetition. Like: <mglPopup (popupOpen)="..." (popupClose)="..." [popupThis]="..." popupThat="..."->. Too many "popup"s as for my taste :)

I agree on your point about "onClick"/"clicked". But i do not see any better alternatives. Also, it seems that both are widely used by libs out there.

If you asked me, i'd go with "clicked" as it's familiar to everyone who uses Angular Material. I'd also change all conflicting names, while keeping the old for some time and deprecating them.

In any case, the final decision is yours. Please make it when you have time.

@dmytro-gokun dmytro-gokun self-assigned this Jul 25, 2020
@jamesrusso
Copy link

I'm being impacted by this too. Right now, my workaround is to simply just look for the lngLat.

onClickMap(evt: MapMouseEvent) {
    if ('lngLat' in evt) { 
       ...
    }
}

@dmytro-gokun
Copy link
Collaborator

@jamesrusso Yes, i plan to fix this at some point. But that will be in the next major release (5.X) since it's a breaking change. Unfortunatelly, at the moment i cannot tell when i will have time to work on this.

@Wykks Wykks closed this as completed in 7b50fd8 Dec 12, 2020
gags88 added a commit to Georadix/ngx-mapbox-gl that referenced this issue Mar 22, 2021
* feat: update to mapbox-gl 1.12.0 (Wykks#253)

* fix: Change many outputs to avoid confusion (fix Wykks#107)

Inspired by @angular/component google-map component

* docs: move docs to repository

* test: fix map test

* showcase: refactor whole app & add doc versionning

BREAKING CHANGE:

Remove undocumented resize API

* chore: update and run prettier

* docs: fix mgl-image example

* feat(mglDraggable): remove marker support

BREAKING CHANGE:

Remove marker support for mglDraggable (use draggable input instead)

* showcase: fix incorrect import (& small layout issue)

* fix(draggable): fix incorrect use of takeUntil

takeUntil should not be used before a switchMap. Using simple subscription pattern instead for consistency

Also fix geolocate output type

* feat: update to mapbox-gl 1.13.0

* test: fix layer test

* chore(release): 5.0.0

* build(deps): bump @mapbox/mapbox-gl-geocoder to ^4.0.0 and add type definitions

Co-authored-by: makspetrov <mp@keatech.com>

* build(deps): use Angular 11 & mapbox-gl 2.0

Co-authored-by: makspetrov <mp@keatech.com>

* chore(release): 6.0.0

* fix(deps): add @types/mapbox__mapbox-gl-geocoder (Wykks#274)

* chore(release): 6.0.1

* fix: declare Position interface for geolocate-control (Wykks#276)

* chore(release): 6.0.2

* fix: Position interface (Wykks#278)

* chore(release): 6.0.3

* build: update @types/mapbox-gl 2.0.3 -> 2.1.0 (Wykks#281)

* chore(release): 6.0.4

* doc: correct README.md on the subject of token being mandatory (Wykks#287)

* build(deps): bump elliptic from 6.5.3 to 6.5.4 (Wykks#288)

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.5.3 to 6.5.4.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.5.3...v6.5.4)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): update dependencies

Co-authored-by: Anthony MacKinnon <anthony@georadix.com>
Co-authored-by: Wykks <contact@wykks.eu>
Co-authored-by: Maks <Nosfit@ukr.net>
Co-authored-by: makspetrov <mp@keatech.com>
Co-authored-by: Dmytro Gokun <dmytro.gokun@gmail.com>
Co-authored-by: Aymen Dhahri <41507665+DroidZed@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants