Locator feature #3803
Locator feature #3803
Conversation
|
😱 awesome! |
| begin | ||
| self.lat, self.lng = coordinates.split(',') | ||
| rescue Exception => e | ||
| puts e.message |
There was a problem hiding this comment.
I think I'd prefer a Rails.logger call instead of puts :)
There was a problem hiding this comment.
True, I used the puts when I was testing it and I forgot to change it XD
|
I'd like to see it in 0.0.3.0 👍 |
|
I think we should merge it right after 0.0.3.0 so that the beta testers can exercise it for a while and then release 0.1.0.0 next ;) |
|
I read "...beta testers can exorcise it for a while..." :P Yes, I agree, we should give this a period of time for live testing for as Am Mittwoch 19 Dezember 2012, 06:47:09 schrieb Jonne Haß:
|
|
Ok, I pulled this into my own develop branch early and it seems whenever i use the locator (it works fine by the way!) i get a security certificate error - it seems something is loaded insecurely from http://nominatim.openstreetmap.org/ during the lookup process. Looking at the openstreetmap API it seems that they do not offer a secure service - so this is something that needs to be looked at. |
|
@marco-gallardo time to pull this in, would you please rebase? :) |
| @@ -158,6 +158,7 @@ group :test do | |||
| gem 'guard-cucumber', '1.2.2' | |||
| gem 'rb-inotify', '0.8.8', :require => false | |||
| gem 'rb-fsevent', '0.9.2', :require => false | |||
| gem 'sinon-rails' | |||
There was a problem hiding this comment.
We got people accidentally running bundle update sometimes, that's why you see strict version requirements everywhere. So lets keep that consistent :)
|
Awesome, I'll apply your observations and do the rebase tonight :) |
|
Edit: Derp ignore that, had precompiled assets laying around |
| @@ -41,6 +41,7 @@ en: | |||
| at_least_one_aspect: "You must publish to at least one aspect" | |||
| limited: "Limited - your post will only be seen by people you are sharing with" | |||
| public: "Public - your post will be visible to everyone and found by search engines" | |||
| near_from: "Near from:" | |||
There was a problem hiding this comment.
Please interpolate the value coming after this, like "Near from: <%= location %>"
There was a problem hiding this comment.
When I do that I get 'http://localhost:3000/stream' instead location's address. I tried renaming 'location' to avoid conflicts but I haven't been able to pass new variable as a parameter in the template file. Is there a special way to accomplish that?
There was a problem hiding this comment.
Weird, we already do this a couple of times and it's working. For example here: https://github.com/diaspora/diaspora/blob/develop/app/assets/templates/reshare_tpl.jst.hbs#L26
There was a problem hiding this comment.
Ok, I'll check that file, thanks.
There was a problem hiding this comment.
Awesome, that worked :)
|
So, only nitpick left is: |
|
Oh, OpenLayers seems to be available as gem: https://github.com/tmikoss/openlayers-rails So I'd prefer to use this instead of copying it over :) |
| setFixtures('<div id="location"></container>'); | ||
|
|
||
| // creates a new Location view with the #location element | ||
| // with this we avoid unnecessarily to call the google API |
|
Same for Sinon, where you already pull the gem in but then don't use it but include sinon again as js file. Actually, where's OpenLayers used? I'm unable to spot it. |
| }); | ||
|
|
||
| app.views.LocationStream = app.views.Content.extend({ |
There was a problem hiding this comment.
I'm still not an expert for our client side JS but shouldn't every view have it's own file so you don't need ctags to find it? :)
|
I'm actually not sure where you use sinon either, so if you could point that out that would be aweseome. |
|
Okay I found one more thing: when resharing a post with an attached location it's missing. I guess if we want to fix that we would need to expose the location via the public get route for posts too and process it when fetching it in case we receive a reshare but never saw the post. |
|
@marco-gallardo what's the status of this ? Can't wait to see it merged !! |
|
Hi guys, I'll try to get all this done by the end of this week, thank you for being so patient. |
|
I guess we rather have to thank you for being so patient ;) |
|
Oh, just a question, how do we set this feature ? Is there a global option to say if we want to share our location or not, or is there a checkbox on the publisher that we have to check each time we post if we want to share, or is there something more precise like "share my location only for limited posts" in the settings ? |
|
Hey @Flaburgan, the idea is to have a link that adds your location to your post when it is clicked. That means that is up to you sharing or not your location. |
|
We need to merge this some times before the release to have time to test it in develop. Any news? |
|
Hi guys, I just rebased develop and pushed the code. It seems to be working pretty fine in my local, however when I ran tests some of them didn't pass. I ran tests also in develop branch and they didn't pass either, so I'm assuming they are not related with this feature (I didn't see my tests crashing). Let me know what you think and sorry for the late response (I've been with a lot of work) |
|
Great! Could you answer my previous questions please? |
|
@MrZyx I just moved that gem into test and development group. Let me know if there are more tweaks I need to do :) |
|
OpenLayers provides that navigator object or...? |
| @@ -204,7 +209,7 @@ | |||
| t.text "data", :null => false | |||
| end | |||
|
|
|||
| add_index "o_embed_caches", ["url"], :name => "index_o_embed_caches_on_url", :length => {"url"=>255} | |||
| add_index "o_embed_caches", ["url"], :name => "index_o_embed_caches_on_url" | |||
|
|
|||
| create_table "participations", :force => true do |t| | |||
| t.string "guid" | |||
There was a problem hiding this comment.
Hm for some reason some indexes vanished here, maybe try a schema:load from develop and then migrate on a fresh database.
|
Hi @MrZyx, what navigator object you are talking about? |
|
osmlocator.js#6 |
|
Oh, that object is provided by the browser; it's a window DOM's object |
|
Okay, then I just don't get what you need openlayers-rails for. |
|
True, we don't need openlayers-rails gem since we are using openstreetmap's api. Deleting that gem. |
|
Hmm, looks like we broke something :/ |
|
Yes, but I think the issue comes from develop branch :( |
|
No, that's green and spec/models/location_spec.rb:12 is the failing spec. |
|
Fixed. |
|
Awesome, add a changelog entry and we're golden from my side. Though a quick second review might not hurt :) |
| it("Show location", function(){ | ||
|
|
||
| // inserts location to the DOM; it is the location's view element | ||
| setFixtures('<div id="publisher_textarea_wrapper"></container>'); |
There was a problem hiding this comment.
you open a div here, but close a container element ... that doesn't look right
There was a problem hiding this comment.
pfff!... fixing it
added marker image for publisher, showing div for address, and added functionality to close it when clicking 'X' image showing marker image in publisher renamed map-marker to locator fixed style for map marker image in publisher added locator.js to get user's location loading google maps api removed unecessary append and showing location next to publisher moved location address inside div and added image to close location styled div location address an image to close location removing location when clicking close image cleaned code cleaned code showing loader while location is being obtained, translated normal js to backbone structure, created locations db stuff, and removing location div when clicking 'share' refactored code; started to move code to backbone refactored js code; moving functionality to backbone structure created address function to make accessible address variable value refactored locator.js showing and removing location div from dom created location; it belongs_to status_message added location model and created association with status_message added hidden field for location address and added respective code on js files to retrieve it on ajax call saving location for status_message (post) removing location when sharing renamed locator backbone view to location, added template, showing location, and saving lat and lng prepared and added template to show location added location to post model in order to have it accessible in backbone retrieving location to show it in template removed console.log XD fixes when removing location cleanind location_address hidden field when location is removed more fixes; showing location when sharing saving location just when it exists created method to retrieve location address just when location was created fixed issue about showing 'Near from' message when there was not any location added style for location cleaned code renamed locator view retrieving lat and lng from locator.js saving lat and lng in location_coords saving lat and lng added style for input location_address removed location_address hidden field; the value will be taken directly from input with the location replaced div with location for input; the user will be able to edit the place avoiding submitting the form when pressing Enter key on new input for location added missed spec file for location model refactored location_view code refactored location_view code cleaned code added sinon library for testing added describes for new publisher's view functions created test for destroyLocation function added test for showLocation publisher view function created test for avoidEnter publisher view function removed unnecessary div Created first test for locations view, added more specs, added Sinon.js, and fixed issue with assets loading locator.js for tests moved location stuff to app/assets moved locator.js and sinon.js to app/assets fixed route for images included locator.js to assets fixed issue when post object is different than StatusMessage; also fixed issue with lat and lng loading Sinon for specs refactoring locator errorGettingposition and start replacing google maps stuff with OSM added OpenLayers JS, osmlocator, and added them into the main js changing the locator from Google to OSM instance changing lat and lng value in the backbone view removing google javascript tag in application layout adding jasmine to locator test and removed locator.js adding jasmine to locator test using OSM adding Jasmine test to OSM locator removed locator.js removed require locator and updated schema fixed js response; added location since we are using OSM Locator we don't need locator-spec test fixed spec for location view; we are not using google maps anymore changed description of osmlocator-spec fixed issue with status-message-location template fixed style for location_address textbox fixed tests for locator moved split function to model created test for location model removed puts added effect for location marker added translations for locator removed conflicting-unnecessary lines that were loading files for specs removed sinon library; using sinon-rails gem removed useless code removed puts; added Rails.logger.error added sinon.js file added specific version of sinon-rails gem improving validations sintax using openlayers-rails gem removed 'google API' text using sinon gem isolating LocationStream view refactored validation getting location when post is a Reshare refactored code fixed aligment for elements under location message improved styling for location message refactored begin-rescue block getting absolute root instead of just the root added address method to retrive address of location removed code from Post model; also added descriptinon why it was removed removed validation when retrieving address; with latest refactorizations we dont need them any more interpolated location; using file in locales fixed width for div of location moved Sinon gem into development and test group fixed method's description added missed indexes updated schema with locations table removed openlayers-rails gem preventing location to be saved if there are not coordinates fixed spec; wrong closing tag
|
Mmh and Travis is red again :( |
|
Weird, I get green on that test in my local :/ |
|
Maybe its just a random failure, lets see :) |
|
Awesome! Thanks for all your patience guys :) |
|
@marco-gallardo Nice! Thanks for your great work! |
|
Thanks |
|
@marco-gallardo congrats! And thank you very much for your nice work! |
|
It's great contributing in such a challenging project guys, congrats to you for this awesome project! |
|
Hmm, the Jasmine failure repeatedly happens on CI, if we only had a clean way to reproduce it :/ |
|
@marco-gallardo what's your Diaspora* handle? I'd like to mention you :) |
|
@Flaburgan it's mgallardo :) |
|
Fantastic. So great to see this finally merged! Wonderful job @marco-gallardo and everyone that has helped him! :) |
|
Thanks very much to @marco-gallardo and everyone who has helped make this feature a reality. I've just tested it, and found one little issue - not exactly a bug, but a small grammatical error, so I've not opened a new issue: in the English version, the text reads 'Near from: [address]'. This isn't correct English. It would be better to read either 'Posted from:' (if it's an exact location) or simply 'Near:' (if you want to give an inexact location). Eg: if I was sitting in a café in the middle of Milano, it might either say 'Posted from: Café X, Milano' or 'Posted from: Milano'. If I was in a village five miles outside Milano, it would be better to say 'Near: Milano'. Either of those is fine, but 'Near from' doesn't read well in English. Many thanks again for all the hard work on this. |
Hi guys, I closed old pull request and opened this new one against develop. I already fixed and removed wrong code.
added marker image for publisher, showing div for address, and added
functionality to close it when clicking 'X' image
showing marker image in publisher
renamed map-marker to locator
fixed style for map marker image in publisher
added locator.js to get user's location
loading google maps api
removed unecessary append and showing location next to publisher
moved location address inside div and added image to close location
styled div location address an image to close location
removing location when clicking close image
cleaned code
cleaned code
showing loader while location is being obtained, translated normal js to
backbone structure, created locations db stuff, and removing location
div when clicking 'share'
refactored code; started to move code to backbone
refactored js code; moving functionality to backbone structure
created address function to make accessible address variable value
refactored locator.js
showing and removing location div from dom
created location; it belongs_to status_message
added location model and created association with status_message
added hidden field for location address and added respective code on js files to retrieve it on ajax call
saving location for status_message (post)
removing location when sharing
renamed locator backbone view to location, added template, showing
location, and saving lat and lng
prepared and added template to show location
added location to post model in order to have it accessible in backbone
retrieving location to show it in template
removed console.log XD
fixes when removing location
cleanind location_address hidden field when location is removed
more fixes; showing location when sharing
saving location just when it exists
created method to retrieve location address just when location was created
fixed issue about showing 'Near from' message when there was not any location
added style for location
cleaned code
renamed locator view
retrieving lat and lng from locator.js
saving lat and lng in location_coords
saving lat and lng
added style for input location_address
removed location_address hidden field; the value will be taken directly from input with the location
replaced div with location for input; the user will be able to edit the place
avoiding submitting the form when pressing Enter key on new input for location
added missed spec file for location model
refactored location_view code
refactored location_view code
cleaned code
added sinon library for testing
added describes for new publisher's view functions
created test for destroyLocation function
added test for showLocation publisher view function
created test for avoidEnter publisher view function
removed unnecessary div
Created first test for locations view, added more specs, added Sinon.js,
and fixed issue with assets
loading locator.js for tests
moved location stuff to app/assets
moved locator.js and sinon.js to app/assets
fixed route for images
included locator.js to assets
fixed issue when post object is different than StatusMessage; also fixed issue with lat and lng
loading Sinon for specs
refactoring locator errorGettingposition and start replacing google maps stuff with OSM
added OpenLayers JS, osmlocator, and added them into the main js
changing the locator from Google to OSM instance
changing lat and lng value in the backbone view
removing google javascript tag in application layout
adding jasmine to locator test and removed locator.js
adding jasmine to locator test using OSM
adding Jasmine test to OSM locator
removed locator.js
removed require locator and updated schema
fixed js response; added location
since we are using OSM Locator we don't need locator-spec test
fixed spec for location view; we are not using google maps anymore
changed description of osmlocator-spec
fixed issue with status-message-location template
fixed style for location_address textbox
fixed tests for locator
moved split function to model
created test for location model
removed puts
added effect for location marker
added translations for locator
removed conflicting-unnecessary lines that were loading files for specs
removed sinon library; using sinon-rails gem
removed useless code