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

mapml-viewer controls attribute/property does not work as an API #758

Merged
merged 31 commits into from
Mar 13, 2023

Conversation

kevinkim31
Copy link
Contributor

@kevinkim31 kevinkim31 commented Feb 16, 2023

  • Fix mapml-viewer & web-map so they can be created via domAPI. closes document.createElement("mapml-viewer") fails #673
  • Fix issue where layer control fails to appear when added programmatically (first layer only). closes Appending a layer to layer-less map does not add layer controls  #672
  • Add domAPI testing
  • Get Testing working for web-map, using:
    • viewer = document.createElement("map", {is:"web-map"})
    • viewer.setAttribute("is","web-map")
  • Fix issue where controls fails to hide/disappear when removed programmatically
  • Add 'controlslist' as an attribute and make it work through using the API
    • implement 'controlslist' as a DOMTokenList
    • implement DOMTokenList as a class to help support it's methods
  • _changeWidth and _changeHeight are causing errors in the console now (on web-map, same thing with mapml-viewer, when map created with width and height attributes it throws an error), as the _container does not get created in the constructor anymore, and is generated afterwards in the connectedcallback

closes #675

kevinkim31 and others added 2 commits February 28, 2023 11:19
Added support for controlslist attribute, which is now represented by a DomTokenList
Decustructed the setControls function to 2 seperate functions: setupControls() - creates the controls and setControls() - which sets the controls visibility using the hidden attribute based on the map's control and controllist attribute
Modified old test to change from looking for deleted controls to now looking for the hidden controls
Added Dom API tests
Removed duplicate test files (domApi.test.js + domApi.html)
TODO - create a DomTokenList class for controlslist?
@AliyanH
Copy link
Member

AliyanH commented Mar 6, 2023

Added a relatively big commit 9717c44 to reimplement setControls(), which now allows the controls and controlslist API to function properly, Changes made:

  • Added support for controlslist attribute, which is now represented by a DomTokenList
  • Deconstructed the setControls function to 2 separate functions:
    • setupControls() - creates the controls
    • setControls() - which sets the controls visibility using the hidden attribute based on the map's control and controllist attribute
  • Modified old test to change from looking for deleted controls to now looking for the hidden controls
  • Added Dom API tests (more tests possibly needed)
    • need to add tests for web-map which currently does not work when the map is created using document.createElement
  • Removed duplicate test files (domApi.test.js + domApi.html)
  • TODO - create a DomTokenList class for controlslist?

@AliyanH
Copy link
Member

AliyanH commented Mar 9, 2023

f360179 - Added DomTokenList for the map's controlsList, removed getter and setter for the controlslist attribute, though controlslist attribute can still be set using setAttribute to emulate the behavior from the video element.

Few notes/questions:

  • jshint is throwing errors for build (when making private properties in the new class), possibly due to Experimental support for class fields jshint/jshint#3139

  • currently the controlsListDomTokenList.js is added directly to the dist dir, is this appropriate?

  • Need to add DomTokenList Specific tests

@AliyanH AliyanH marked this pull request as ready for review March 9, 2023 16:40
@AliyanH AliyanH requested a review from prushforth March 9, 2023 16:40
to use that in mapml-viewer not quite working for reasons not yet clear.

Discardable code fwiw
Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

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

Just a few comments

src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
Change attributeChangedCallback for controls, remove set/getAttribute
@prushforth
Copy link
Member

LGTM, merge when you're happy with it.

@AliyanH AliyanH merged commit 569983e into Maps4HTML:main Mar 13, 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