Size spec added and new template added using BlurredLocation object #7

mridulnagpal · 2017-05-24T19:40:00Z

No description provided.

mridulnagpal · 2017-05-24T19:47:24Z

@jywarren Added a new option for our object and corresponding test. Also made a new template which works with the object. And integrated all js files to a main distribution file. Please review

mridulnagpal · 2017-05-24T22:04:44Z

README updated with options section, will complete it as I write new methods.

jywarren · 2017-05-25T00:13:00Z

Sounds great -- shall I merge this, then? Were you able to check off items on the planning issue at #1 ?

@aspriya - take a look at @mridulnagpal's work here in working with a separate self-contained library. Related, structurally, to your work on publiclab/inline-markdown-editor!

jywarren · 2017-05-25T00:13:53Z

README.md

+
+| Methods | Use | Usage (Example) |
+|---------|-----|-----------------|
+|getLat   | Used to get the current latitude of the center of the map. | blurredLocation.getLat() //This would return the value in numerics |


hmm, am I missing getSize() here? just checking!

Yeah I've added that. I think this looks good to merge now. What do you say? @jywarren

jywarren · 2017-05-25T01:16:22Z

README.md

+|getLat   |Used to get the current latitude of the center of the map.|  blurredLocation.getLat() //This would return the value in numerics|
+|getLon|Used to get the current latitude of the center of the map|blurredLocation.getLon() //This would return the value in numerics|
+|goTo   |Takes in three parameters, namely latitude, longitude and zoom. Will set the center of map to co-ordinates input.|blurredLocation.goTo(44.51, -89.99, 13) //Will set center of map to (44.51,-89.99) with zoom set as 13|
+|getSize   |Used to get the current size of the map, namely the width and height.| blurredLocation.getSize() //This would return the size of the map|


This looks good, and I assume it's needed for some of the other functions. But let's keep in mind what is essential API that cannot be provided by direct access to the map, such as a native Leaflet call (not sure but probably map.getSize()?) Can this be done here without a redundant wrapper, or is getSize really necessary for the API? Thanks!

I don't think it would be necessary, should I remove it then?

mridulnagpal · 2017-05-25T18:01:14Z

If you could merge this I have an upcoming issue which includes file added in this PR.

jywarren · 2017-05-25T20:47:00Z

src/object.js

-        map.setView([geometry.lat, geometry.lng],default_zoom);
-        return geometry;
-      }
+    if(!map) {


This is great, but lets set map via the constructor options, like new BlurredImage(options) where options is like:

function BlurredLocation(options) { options = options || {}; options.map = options.map || L.map('map'); options.location = options.location || someDefault; }

Yeah that looks neat.

mridulnagpal · 2017-05-26T15:05:43Z

@jywarren Please review made the changes.

mridulnagpal · 2017-06-05T06:13:20Z

@jywarren Made changes please have a look

jywarren · 2017-06-09T19:57:33Z

Gruntfile.js

@@ -32,7 +32,7 @@ module.exports = function(grunt) {
        uglify: {
          my_target: {
            files: {
-              'dist/Leaflet.BlurredLocation.js': ['src/location_tags.js', 'src/locationForm.js', 'main.js']
+              'dist/Leaflet.BlurredLocation.js': ['src/location_tags.js', 'main.js']


Hmm, so here, are you concatenating these files into dist/Leaflet.BlurredLocation.js, and how does this work before or after browserify compiles into dist/Leaflet.BlurredLocation.js? Don't you want Browserify to do the compiling, then uglify the output? I'm actually not that familiar with uglify - is that typically run after Browserify?

jywarren · 2017-06-09T19:58:30Z

Gruntfile.js

+            separator: ';',
+          },
+          dist: {
+            src: ['node_modules/jquery/dist/jquery.min.js', 'node_modules/leaflet/dist/leaflet.js', 'src/object.js'],


Then concat also seems redundant with Browserify and uglify - which are we using to compile? It looks liek concat, based on line 76, but maybe we should be using Browserify, no?

Take a look at the Browserify example in PublicLab.Editor -- it'll be helpful! or in https://github.com/publiclab/inline-markdown-editor/blob/master/Gruntfile.js#L55 -- a bit simpler!

mridulnagpal · 2017-06-13T04:06:50Z

Sorry I was out of town, will look into it right now. Thanks

…

On 12-Jun-2017 10:21 PM, "Jeffrey Warren" ***@***.***> wrote: Had a chance to look at my comments? Thanks for your hard work, Mridul! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AVULlKzYfl_Sr7rqE0OVDIj9F-aK-0gbks5sDWykgaJpZM4Nlk0I> .

mridulnagpal · 2017-06-13T16:29:16Z

Please have a look, tried to make it like the inline editor

jywarren · 2017-06-14T15:08:33Z

src/object.js

+};
+
+  function geoLocateFromLatLng(lat,lng) {
+    var lat = document.getElementById(lat);


Can we separate the interface from the underlying code here? So, geoLocateFromLatLng would accept just two decimals, and when it's called, we do, for example, geoLocateFromLatLng($('lat').value,$('lon').value) -- that way it's more re-usable? We could also have a separate mini function like getLatLonFromElements(latEl,lonEl) or even a wrapper like geoLocateFromElements(latId,lonId) which then runs geoLocateFromLatLng() inside itself. See how this modularity can result in simpler and more re-usable code, and simpler tests too?

Also - once this big PR is merged, we can hopefully limit ourselves to each PR dealing with one isolated method, and its matching test. That way each PR can be reviewed quickly and easily by comparing the single method to the test that describes its intended use and output. Make sense?

Yeah sure, I was also thinking of doing it like we used to do it in the main repo, by creating different branches for different issues. Also the function I would make which accepts two decimals and pan the map to that location is already present, i.e panMap().

jywarren · 2017-06-14T15:10:33Z

Going well! I think as we go we'll want, as much as possible, to be isolating the code that touches HTML/UI from the underlying functional code. Maybe even breaking it into subfolders like /src/ui/ and /src/core/ -- though we can do that at another time.

Also -- what's the difference between /dist/built.js and /dist/Leaflet.BlurredLocation.js?

mridulnagpal · 2017-06-15T14:41:04Z

@jywarren I made the build.js file for testing purposes, once it works okay I will replace it. Currently the example uses built.js only

mridulnagpal · 2017-06-15T15:21:22Z

@jywarren Just started on the lines you had suggested. Made /src/core and /src/ui and grunt compiles them accordingly. Please have a look

jywarren · 2017-06-15T15:22:51Z

Re build.js, so they will be redundant? Can you please open an issue to merge them so we don't forget? Thanks.

jywarren

OK -- this is really looking good, it's much more readable and i'm so happy it's compiling using browserify.

I've made a number of requests, some are renamings so that when we write tests for each method, the name of the method really clearly states the (one) thing it's supposed to do. By making methods each do just one thing, we can more easily test them.

I also asked for a couple of them to be rewired to reduce redundancy. If you can run through and make these changes today, that would be great and we can move on to the next step!

Great work and thank you! You're really learning a lot here about architecting a library and it shows!

jywarren · 2017-06-15T15:29:19Z

Gruntfile.js

-            files: {
-              'dist/Leaflet.BlurredLocation.js': ['src/location_tags.js', 'src/locationForm.js', 'main.js']
+                src: ['node_modules/jquery/dist/jquery.min.js', 'node_modules/leaflet/dist/leaflet.js', 'src/core/object.js'],
+                dest: 'dist/built.js'


Yes, let's build to 'dist/Leaflet.BlurredLocation.js' here -- thanks!

jywarren · 2017-06-15T15:35:06Z

src/core/object.js

+    setTimeout(function () {
+      var str = input.value;
+      var loc = str.split(' ').join('+');
+      $.getJSON("https://maps.googleapis.com/maps/api/geocode/json?address=" + loc + "&key=AIzaSyDWgc7p4WWFsO3y0MTe50vF4l4NUPcPuwE", function(data){


Can't this use the geocode function on line 26?

Also let's change the name to panMapToGeocodedLocation() maybe - since we're doing more than just geocoding, we're moving the map too.

Please have a look

jywarren · 2017-06-15T15:37:15Z

src/core/object.js

+    var lat = document.getElementById(lat);
+    var lng = document.getElementById(lng);
+
+    lat.addEventListener('change blur input', function() {


if you define this function as a named function like function panIfValue() {} -- just within the scope of geoLocateFromLatLng(), you can reduce code.

Please have a look

jywarren · 2017-06-15T15:37:33Z

src/core/object.js

+  });
+};
+
+  function geoLocateFromLatLng(lat,lng) {


Again here, maybe panMapFromLatLng()?

er, more specifically -- panMapWhenInputsChange(latId, lngId)?

jywarren · 2017-06-15T15:39:21Z

src/core/object.js

+    map.panTo(new L.LatLng(lat, lng));
+  }
+
+  function getLocationFromMap(lat, lng) {


this needs to be getPlacenameFromCoordinates()

jywarren · 2017-06-15T15:40:24Z

src/core/object.js

+    $.getJSON("https://maps.googleapis.com/maps/api/geocode/json?latlng="+lat+","+lng, function(data) {
+      if (data.results[0]) {
+        var address = data.results[0].formatted_address;
+              $("#location").val(address);


And here, we need this method to return the address, not reference specific DOM elements -- the method should just do one thing, and when we use the method, we can decide what to do with the output. That makes this much easier to test and makes for more modular code!

I have made it return only the location but it doesn't return the location for some reason can you please have a look

jywarren · 2017-06-15T15:41:42Z

src/core/object.js

+    });
+  }
+
+  function getLocation(checkbox) {


let's call this panMapByBrowserGeocode() -- sound good?

jywarren · 2017-06-15T15:56:16Z

So exciting -- this is really coming together and you can see the codebase a whole becoming more clear, more readable, and more organized -- great work!

mridulnagpal · 2017-06-15T20:04:10Z

@jywarren Made some changes please have a look

jywarren

OK, one more request (i promise!) -- see how I've proposed how to distinguish between internal private methods, and an external API that can be accessed like blurredLocation._methodName_() ? This helps us organize what's for internal use (internal scope) and what's "exposed" to the outside for anyone to use.

jywarren · 2017-06-15T21:24:02Z

examples/index.html

+
+  <script>
+    var blurredLocation = new BlurredLocation();
+    panMapToGeocodedLocation('geo_location');


Here, let's use this as an internal method to blurredLocation -- like blurredLocation.panMapToGeocodedLocation(...) -- i'll show you how, below. Same with panMapWhenInputsChange.

jywarren · 2017-06-15T21:24:45Z

src/core/object.js

+    }
+
+    this.addGrid = options.addGrid;
+}


If we move this to include all the below functions, we can make them all methods of blurredLocation.

move this }, i mean

jywarren · 2017-06-15T21:27:46Z

src/core/object.js

+        }
+    }
+  }
+


So right here, where we move the close-curly-bracket for the constructor of function BlurredLocation(options) -- just before the } we can add:

return { getPlacenameFromCoordinates: getPlacenameFromCoordinates, panMapByBrowserGeocode: panMapByBrowserGeocode, //... and so on }

That way we are returning all those functions as an API -- and they're all collected in once place so we know exactly what the outwardly promised API is. Does this make sense?

mridulnagpal · 2017-06-16T05:52:22Z

@jywarren Made the methods internal please have a look

jywarren

This is great. Please do a followup with consistent indents, but it's really great to see such progress! Congrats on this really big PR!

mridulnagpal added 3 commits May 25, 2017 00:30

new example using object added

f6921cb

size spec added

d7696ad

small tweaks

d7a4617

mridulnagpal added 5 commits May 25, 2017 03:27

README updated

b78d5ea

README updated

c29e827

Update README.md

138b289

Update README.md

d05e6ef

Update README.md

ab651be

jywarren reviewed May 25, 2017

View reviewed changes

README updated

f70fc71

jywarren reviewed May 25, 2017

View reviewed changes

mridulnagpal added 3 commits May 25, 2017 07:04

README updated

42c8f43

README updated

0124158

Size removed

b7b9ac7

mridulnagpal mentioned this pull request May 25, 2017

Developing a button to display current latitude and longitude the current position of map is pointing to #9

Closed

5 tasks

jywarren requested changes May 25, 2017

View reviewed changes

options added

1a486dc

mridulnagpal added 2 commits June 5, 2017 11:22

object.js changed

b62fa04

merge conflict

51d742c

mridulnagpal added 3 commits June 5, 2017 12:12

addgrid working

4c0c13a

README updated

32cb2fb

tweaks

363190f

jywarren reviewed Jun 9, 2017

View reviewed changes

mridulnagpal added 3 commits June 13, 2017 21:19

changes in Gruntfile

8bfa37c

js file updated

0af7deb

renaming examples

655845e

jywarren requested changes Jun 14, 2017

View reviewed changes

mridulnagpal added 2 commits June 15, 2017 20:14

Test passing

f1d554e

Structuring the library

81efadc

jywarren mentioned this pull request Jun 15, 2017

Delete location_tags.js #11

Closed

jywarren requested changes Jun 15, 2017

View reviewed changes

Restructuring dist/built.js

e345dca

mridulnagpal added 6 commits June 15, 2017 21:30

Renaming and using geocode function inside of panMapToGeocodedLocation()

0c49afb

Removing redundancies

54cb90e

Renaming function

1bbab6b

Little tweaks to clean code

c289926

tweaks

f5fd39a

tweaks

9b3507f

jywarren requested changes Jun 15, 2017

View reviewed changes

mridulnagpal added 2 commits June 16, 2017 11:09

Making global methods internal

d51c63c

Test passing

642ab51

jywarren approved these changes Jun 16, 2017

View reviewed changes

jywarren merged commit 4b06a1d into publiclab:master Jun 16, 2017

jywarren mentioned this pull request Jun 16, 2017

Begin compiling source files together using Browserify #10

Closed

Size spec added and new template added using BlurredLocation object #7

Size spec added and new template added using BlurredLocation object #7

Conversation

mridulnagpal commented May 24, 2017

mridulnagpal commented May 24, 2017

mridulnagpal commented May 24, 2017

jywarren commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulnagpal commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulnagpal commented May 26, 2017

mridulnagpal commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulnagpal commented Jun 13, 2017 via email

mridulnagpal commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jun 14, 2017

mridulnagpal commented Jun 15, 2017

mridulnagpal commented Jun 15, 2017

jywarren commented Jun 15, 2017

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jun 15, 2017

mridulnagpal commented Jun 15, 2017

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulnagpal commented Jun 16, 2017

jywarren left a comment

Choose a reason for hiding this comment