Integration of new map module of PublicLab.editor . #4608
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
Just a few requests and a suggestion to aim for some closer integration. Thanks @sagarpreet-chadha !!!
| $("#map_content").toggle() ; | ||
| }) ; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Down on line ~257, could we insert some params from the tags if they exist? Like, if there are already lat:___ and lon:____ tags in format /post?tags=one,two,lat:90,lon:-90 we could detect them and insert them here? We can just supply them in format:
editor = new PL.Editor({
lat: 90,
lon: -90,
...for now, and develop the detection and response code for it later in the Editor itself. How does that sound?
There was a problem hiding this comment.
Awesome ! I guess we are trying to solve the case when we edit a document , right ?
Currently default location on map will be shown and not the existing one .
There was a problem hiding this comment.
I think we might want to do either edit or create new -- both will open with this template. So if we add this option, we'll be able to insert either pre-populated ones from a link, or the default ones if it's editing an existing node. How does that sound?
There was a problem hiding this comment.
Makes sense 😄 !
| @@ -12,6 +12,10 @@ | |||
| <meta name="author" content="Public Lab contributors" /> | |||
| <link href="https://<%= request.host %>/feed.rss" rel="alternate" type="application/rss+xml" title="Public Lab research" /> | |||
|
|
|||
|
|
|||
| <script src="https://maps.googleapis.com/maps/api/js?libraries=places&language=en&key=AIzaSyAOLUQngEmJv0_zcG1xkGq-CXIPpLQY8iQ"></script> | |||
There was a problem hiding this comment.
Hi @jywarren ! Instead of adding the if condition in _footer.html.erb , can i add that if condition here ?
When used in footer , the Google API gave - google not found error and when adding an explicit script tag to the page - it gave - 'google api used two times , may give unexpected error' .
So i finally removed the google API script everywhere AND kept it here . What do you think ?
There was a problem hiding this comment.
Sure, that sounds good. Thanks!
|
@publiclab/reviewers @publiclab/plots2-reviewers @jywarren ...i guess we can ignore the code-climate issue ?
|
|
@gauravano , @SidharthBansal ...what do you think ? Any pointers on how to reduce cognitive complexity OR it is currently easy to understand ? |
|
I think we can ignore this. Your work looks good to me. |
|
Testing your PR on |
|
Hey @sagarpreet-chadha, I also think we can ignore the cognitive complexity issue. Let's see how map module is looking in unstable. Thanks! |
|
A couple more comments above, but this looks cool! I think we ought to incorporate the lat/lon options before merging, but then we'll be ready. We can, in the template, only insert the values Great!!! |
|
Looks great. Referencing publiclab/PublicLab.Editor#211 and merging! Follow up will be to get the editor to detect the lat/lon initial data we pass. Thanks!! |
* integration done * changes done * missing spinner dependency resolved * codeclimate resolve * coordinate exist condtions added
Integration of new map module of PublicLab.editor .
@jywarren ...kindly review this 😄 !