-
Notifications
You must be signed in to change notification settings - Fork 107
Conversation
… reading an object hash via scope function binding to $scope.mapOptions; removed mapOptions validations
…s to esriMap directive
Additional to do:
|
…-options"; added link from docs example to test page with inline JSON style
…e(); altered some wording on the additional-map-options docs and test pages
Go @jwasil , it's your birthday 🍰! New examples/test page look great. It occurs to me that one of the two should probably load a web map to demonstrate the capabilities of passing the options when loading a web map. You can either add that to your list above if you want to handle it in this PR, or create a new issue for the backlog. |
} | ||
}; | ||
|
||
/*esriLoader.require(['esri/geometry/Extent', 'esri/SpatialReference'], function(Extent, SpatialReference) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomwayson thank you for reviewing what is in here so far! You're completely right about the commented out code showing up in the front-facing code snippet. That's just sitting in there temporarily until I move it out to a new example/test page that loads in an extent and infoWindow from the mapOptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! I'll wait for all the checkboxes to be ticked before final review. Thanks!
@tomwayson good point on showing a webmapId and mapOptions jiving together. I've updated the to do list above. Feel free to modify the to do list if anything else comes to mind. Thanks ✋-5 |
…ne webmap-id and inline map-options; updated related example docs page with more descriptive info
… creation based on mapOptions in esriMap directive, including adjustments to how extent-changes are handled with different map projections; replaced console.log with $log.log in esriMap directive; updated "Simple Map" subtitle as per issue #70;
Hey @tomwayson feel free to start digging in! The new "Advanced Map Options" example page is up, which required changes in a few spots to the esriMap directive; the most recent commit attempts to compartmentalize those changes. Looking forward to your thoughts on how the Extent and Popup are constructed, particularly the srcNodeRef param on the Popup. Thanks! |
only needed for debugging, should not be in release also, I don't like to use $log, see: http://stackoverflow.com/questions/24185847/why-use-angulars-log-instead-of-console-log#comment41761510_24185961
does not use binding to center/zoom this caused errors when map extent-change handler tried to update center, so I had to modify that a bit also, updated descriptions for all examples
Great work @jwasil! I made a few minor changes. I really appreciate you digging into the extent and info window options on top of everything else. I like the way you check if the extent needs to be created or not. I'm actually still playing w/ a few different things like checking for scope properties like basemap, center, and zoom before checking for the web map id, etc. I just wanted to get this thing merged b/c we've gone so far beyond the original bug fix and we can handle any future changes under new issues if need be. |
Awesome, @tomwayson! Thanks for reviewing this, providing advice, and doing some additional work on this PR. I just noticed the "Additional Map Options" test page is not linked to properly. If it doesn't step on something you're looking into, I can do some quick changes to link to the unserved git source. |
^ Please disregard my confusion b/w additional map options and no basemap above. This was written before ☕! The only thing is how to handle the link to the additional map options test page. |
Good catch. Please look into why that's not working - the link is right, I just don't know why the test file didn't get deployed. |
done and deployed to gh-pages as well |
@tomwayson please take a look. (Hopefully) resolves #65. Please see commit comments for high-level changes.