Skip to content

map-feature bug fixes - #841

Merged
AliyanH merged 11 commits into
Maps4HTML:mainfrom
AliyanH:map-feature-bug
May 19, 2023
Merged

map-feature bug fixes#841
AliyanH merged 11 commits into
Maps4HTML:mainfrom
AliyanH:map-feature-bug

Conversation

@AliyanH

@AliyanHAliyanH commented May 1, 2023

Copy link
Copy Markdown
Member
// $0 represents the map-feature element$0.addEventListener("click",(e)=>{e.originalEvent.stopPropagation();});
  • add support for keyup/keydown event on map-feature
  • Add Tests

closes#838

@AliyanH
AliyanH requested a review from prushforthMay 5, 2023 17:36

@prushforthprushforth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good simplifications, and they seem reliable.

Comment threadsrc/map-feature.js
@prushforth

Copy link
Copy Markdown
Member

I would have liked to get rid of the click(ev), focus(ev) and blur(ev) parameter, as it seems non-standard, but perhaps that's not possible because we have got leaflet handling the underlying clicks and displaying popups and so on.

Comment threadsrc/map-feature.js
@AliyanH

AliyanH commented May 15, 2023

Copy link
Copy Markdown
MemberAuthor

noticed another issue just now that I will work on, removing a map-feature when it is unchecked does not work (can be handled in a separate issue).

@AliyanH
AliyanH requested a review from prushforthMay 18, 2023 19:36
@AliyanH

Copy link
Copy Markdown
MemberAuthor

I think this is good to go @prushforth , just a few changes I added:

  • removed non-standard parameters for click, focus, blur methods (based on suggestion by @prushforth)
  • Added support for keyup/keydown on map-feature

@prushforth

Copy link
Copy Markdown
Member

Would you mind fixing this bug in this PR? If that doesn't make sense, feel free to merge this, it looks and works well.

@prushforth

Copy link
Copy Markdown
Member

I like the test for preventDefault that displays a different thing than the popup for the polygon, good idea there. It tests the API and not just our API; that's exactly what we need to do more of.

@AliyanHAliyanH linked an issue May 19, 2023 that may be closed by this pull request
@AliyanH
AliyanH merged commit 32a7a40 into Maps4HTML:mainMay 19, 2023
@AliyanH
AliyanH deleted the map-feature-bug branch May 19, 2023 19:38
prushforth pushed a commit to prushforth/MapML.js that referenced this pull request Jun 23, 2023
* remove map-feature bug
* fix/update Event implementation
* stopPropagation attempt
* Finalize map-feature Event handling
* revert index.html
* Add test for stopPropagation
* update observed attributes + add test for click method
* remove non-standard event parameter for click,focus,blur methods
* add support/link for keyup+keydown event
* close popup on zoom to here link, closesMaps4HTML#855
Sign up for freeto 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.

Popup remains open after zoom to here link is clicked Removing map-feature to leave an empty layer- throws an error

2 participants

@AliyanH@prushforth