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

Develop <map-feature> custom element #801

Merged
merged 56 commits into from
Apr 20, 2023
Merged

Conversation

yhy0217
Copy link
Contributor

@yhy0217 yhy0217 commented Mar 15, 2023

Closes #528

  • make mapFeature element interactive

  • when mapFeature is removed from DOM, the associated g and path elements are removed

  • when mapFeature (and its children elements) is re-added / updated in the DOM, the associated g and path elements are updated dynamically

  • provide methods and APIs as suggested in Implement <map-feature> custom element #528

  • .click(): simulate a click event by default; customizable to users

  • .focus(): simulate a :focus state; show the focus rectangle and change the current document.activeElement by default; customizable

  • .mapml2geojson(): return geojson representation of geometry

  • .extent: return feature extent

  • .zoomTo: zoom to the zoom attribute value of map-feature; if the attribute is not present but a max attribute is set, zoom to the max attribute value; otherwise, zoom to the maximum zoom level that can show the feature completely

  • .pasteFeature (method of layer- element): allow users to paste new feature to a selected layer as a string or an object

  • create and attach a shadow root for the layer- elements with an src attribute

  • attach shadow and migrate the elements in mapml file to shadow when a layer- element is loaded

  • when a layer- element's src attribute is changed (added/removed/updated), content in shadow will change accordingly and dynamically

  • make src take precedence over the inline content; inline content can still work as a fallback when the layer's src attribute is removed

  • create tests for mapFeature custom element and shadow root of layer- elements

  • Add "zoom to here" link in pop-up; localize, closes Localize "Zoom To Here" link in popup mapml-extension#61

  • add min and max attributes to define the zoom value range that feature should be visible

  • close <map-feature> with no child <map-properties> is not focusable, does not display <map-featurecaption> #803

  • close Projection negotiation / layer src loads the remote mapml resource twice #807

  • close Popup keyboard navigation bug #507

Copy link
Member

@AliyanH AliyanH left a comment

Choose a reason for hiding this comment

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

Just exploring all the different functionality you added so far, and a few comments about them. Looking great so far!

src/map-feature.js Outdated Show resolved Hide resolved
src/map-feature.js Outdated Show resolved Hide resolved
src/map-feature.js Outdated Show resolved Hide resolved
src/map-feature.js Show resolved Hide resolved
src/map-feature.js Outdated Show resolved Hide resolved
@prushforth

This comment was marked as resolved.

src/layer.js Outdated Show resolved Hide resolved
@AliyanH

This comment was marked as resolved.

@yhy0217 yhy0217 marked this pull request as draft April 12, 2023 13:14
@prushforth
Copy link
Member

Bug report: if you load the index.html with twohats guy, and change his coordinates to 0 0 (any value that will place him out of the viewport will do it), then pan over to him and click on his placemark, you get this in the console:

mapml.js:7168 Uncaught TypeError: Cannot read properties of undefined (reading 'path')
    at i._handleFocus (mapml.js:7168:51)
    at SVGGElement.o (leaflet-src.js:2799:15)

@yhy0217
Copy link
Contributor Author

yhy0217 commented Apr 18, 2023

mapml.js:7168 Uncaught TypeError: Cannot read properties of undefined (reading 'path')
    at i._handleFocus (mapml.js:7168:51)
    at SVGGElement.o (leaflet-src.js:2799:15)

I got this error message on the main branch too, I thought that may be the case because the _handleFocus function appears to be intended for keyboard events, while in this case the event is an instance of MouseEvent. Btw here is another one I got:

Uncaught TypeError: L.control.locate is not a function
    at i.onAdd (mapml.js:6192:38)
    at i.addTo (leaflet-src.js:4929:44)
    at MapViewer._createControls (mapml-viewer.js:369:55)
    at MapViewer._createMap (mapml-viewer.js:269:12)
    at MapViewer.connectedCallback (mapml-viewer.js:166:12)
    at mapml-viewer.js:1035:23

@AliyanH AliyanH closed this Apr 18, 2023
@AliyanH AliyanH reopened this Apr 18, 2023
@AliyanH
Copy link
Member

AliyanH commented Apr 18, 2023

and here is another one I got:

This one is likely because you need to run "npm i" before.

@yhy0217 yhy0217 marked this pull request as ready for review April 19, 2023 00:07
@prushforth
Copy link
Member

prushforth commented Apr 19, 2023

@yhy0217 this is looking really solid today. I noticed that when rendering features via the extension (if you put a build of this branch in the extension), you can't visually delete features that are extension-rendered from the layer-.shadowRoot (within the console) i.e. it doesn't remove the linked <g> element when you do that. This is likely an issue with the extension, but maybe you could have a look and comment, if you see anything.

image

Copy link
Member

@AliyanH AliyanH left a comment

Choose a reason for hiding this comment

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

We can remove mousedown event from the feature group, instead of omitting mouse event on the _handleFocus method. If I understand correctly _handleFocus is only handling keyboard events and nothing related to mouse events?

src/mapml/layers/FeatureLayer.js Outdated Show resolved Hide resolved
src/mapml/features/featureGroup.js Outdated Show resolved Hide resolved
src/mapml/features/featureGroup.js Outdated Show resolved Hide resolved
yhy0217 and others added 2 commits April 19, 2023 16:00
Co-authored-by: Aliyan Haq <55751566+AliyanH@users.noreply.github.com>
Co-authored-by: Aliyan Haq <55751566+AliyanH@users.noreply.github.com>
@yhy0217
Copy link
Contributor Author

yhy0217 commented Apr 19, 2023

I noticed that when rendering features via the extension (if you put a build of this branch in the extension), you can't visually delete features that are extension-rendered from the layer-.shadowRoot (within the console) i.e. it doesn't remove the linked <g> element when you do that.

I believe the "link" / "connection" between the map-feature element and <g> element is set up properly when the extension renders the map, so the codes should work as our expectation in this process:
image

I think the issue might be related to the webcomponent-bundle.js file. It seems that when we use the extension to render XML files, it may "clone" the custom elements we defined (layer-, map-feature, etc.) and attach the cloned element to the DOM instead of the ones that we create. If you remove the <layer- > element in the experiment (https://maps4html.org/experiments/shared/restaurants/restaurants.xml), you will notice that the layer is still present, just as we observed with <map-feature>. These "cloned" elements in the DOM do not have accessibility for any properties that we set (e.g. <layer- >._layer):
image

So even though the links between and feature elements that we created are properly set, they are not "reflected" to the "cloned" elements that we can handle in DOM.

@prushforth
Copy link
Member

This is spectacular work. I think you're 98% done, let's discuss in the morning. The only challenge that I can see is making the "Zoom to here" link DRY; if we could find a way to do that I would be VERY happy. I haven't had a chance to use / test the onfocus, onblur and onclick handler methods or event handlers yet. I would like to set those up as an experiment for future reference, lest we forget they exist. I'll continue with the documentation updates.

@yhy0217 yhy0217 merged commit 930b852 into Maps4HTML:main Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants