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

Add location tag for notes #650

Closed

Conversation

lalithr95
Copy link
Member

@lalithr95 lalithr95 commented Aug 6, 2016

This PR adds integration for Notes allowing user to tag location with content and also taking care of privacy concerns.

Make sure these boxes are checked before your pull request is ready to be reviewed and merged. Thanks!

  • All tests pass -- rake test:all
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request are descriptively named
  • if possible, multiple commits squashed if they're smaller changes
  • reviewed/confirmed/tested by another contributor or maintainer
  • schema.rb.example has been updated if any database migrations were added

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

@lalithr95 lalithr95 changed the title Add location tag for notes - WIP (tests pending) Add location tag for notes Aug 11, 2016
@lalithr95
Copy link
Member Author

Screenshots:

New note:
2

Note Display location with map:
4

Edit without privacy enabled:
5

Edit with Privacy enabled:
1

@lalithr95
Copy link
Member Author

@jywarren @ebarry @steviepubliclab

This PR is ready for merge. All tests are passing. Users will be able to add location to their content and also take care of privacy concerns by truncating exact location. LocationTag will be a generic model which can also be extended to any other models in future.

Review please 👍 🌏 🎉

@ebarry
Copy link
Member

ebarry commented Aug 11, 2016

HI Lalith,
Looks great! Great work!
I see the hexagon on the map!
Now, I do love hexagons, but in the most recent conversation, it seemed
like the best path for obscuring location would be to drop a couple
significant figures off the lat/lng, and display as a square of the
graticule. Could be stored easily as a geohash http://geohash.org/
Also, i think @jywarren identified a specific problem with the hexagon
libraries, that each hexagon does not have a unique ID.
Could you please comment on these points?
Thank you!

+1 336-269-1539
@lizbarry http://twitter.com/lizbarry

On Wed, Aug 10, 2016 at 11:35 PM, Lalith Rallabhandi <
notifications@github.com> wrote:

@jywarren https://github.com/jywarren @ebarry
https://github.com/ebarry @steviepubliclab
https://github.com/steviepubliclab

This PR is ready for merge. All tests are passing. Users will be able to
add location to their content and also take care of privacy concerns by
truncating exact location. LocationTag will be a generic model which can
also be extended to any other models in future.

Review please 👍 🌏 🎉


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#650 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAJ2n3J-wWkC3hDsCTEQfbQaj1IGAwPLks5qepiLgaJpZM4JeY1Z
.

@lalithr95
Copy link
Member Author

@ebarry I'm obscuring lat and long values, https://github.com/publiclab/plots2/pull/650/files#diff-b9551b46e6321bfb977e1f52104c5768R136 We store individual lat, long columns in the database. GeoHash is something which brings all attributes, (12°0.000' 45°0.000'). This needs to change column to string which breaks geo location in Profile pages. My opinion would be use same way in both interfaces (profile, content) so that users don't get confused.

@ebarry
Copy link
Member

ebarry commented Aug 11, 2016

Hi Lalith,
I will leave it to @jywarren to advise if there's any benefit to adding a
geohash column type string.

I see your point about not confusing users by having two different user
interfaces for adding location.
I will reach out to two user interface designers in the community to advise
on this: Hi there @weatherpattern and @annhchen, can you please take a look?

My* pressing concern* is user privacy. Having privacy on by default -- and
showing people exactly how other people will see their location based upon
how they add it is critical.

+1 336-269-1539
@lizbarry http://twitter.com/lizbarry

On Thu, Aug 11, 2016 at 9:56 AM, Lalith Rallabhandi <
notifications@github.com> wrote:

@ebarry https://github.com/ebarry I'm obscuring lat and long values,
https://github.com/publiclab/plots2/pull/650/files#diff-
b9551b46e6321bfb977e1f52104c5768R136 We store individual lat, long
columns in the database. GeoHash is something which brings all attributes,
(12°0.000' 45°0.000'). This needs to change column to string which breaks
geo location in Profile pages. My opinion would be use same way in both
interfaces (profile, content) so that users don't get confused.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#650 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAJ2n00M7LytJMDxn-4EwMwQInPdcC3zks5qeyocgaJpZM4JeY1Z
.

@jywarren
Copy link
Member

I'll need to do a pretty close review of this, and I'm not sure I'll have time before my flight on Sunday, but I'll check out a version to review in flight.

It's great to see this work, and this may be one of those PRs where we have to iterate a few times before merging.

Thanks for your hard work and keep going on the UI stuff -- great idea Liz.

@jywarren
Copy link
Member

OK, i'm not done reviewing but I have a few clarifications and requests:

  1. After truncating lat/lon, I think we should show exactly what we've done via the UI to the person filling it out -- that is, show the lat/lon rectangle we've "downscaled" to. However, I don't think we can access the precision of a value in the database -- we can write in a lower precision value, but it will be returned with whatever significant digits the database returns, no? Extra zeroes at the end? Will there be any drift if we do math with it? Perhaps we need to store an extra "precision" integer, a # of significant digits? Though I see we store "location_privacy" -- is this a boolean? maybe we should make it an integer so we have some options for how much we truncate.
  2. 'obscure location' instead of 'obscure my location' -- so it's standard across interfaces
  3. After "obscure location" show a link Learn how and why leading to a wiki page called /wiki/location-privacy
  4. If window.google is not returned, we should display an error to the user in the interface. Maybe see how input form errors are displayed in the Bootstrap framework.

@jywarren
Copy link
Member

Also, what does the Map button do when you click it?

@jywarren
Copy link
Member

And, I see that when I drag the map, the hexagon (which will be a rectangle) is not updated. The center of the map should be the "point" location which is downsampled, so when you drag the map, the "active" zone should be updated.

Because of this, it's important to show more than just the active zone -- the active one should be highlighted in some way -- a red border, maybe -- and the inactive ones should be grey. This way it's clear that the active zone is just whichever one the map center falls inside. Make sense?

@jywarren
Copy link
Member

@ebarry, if you have a moment to include this in a sketch that would be very helpful! I will be behind on things for a few days as I catch up after my trip.

@@ -99,6 +99,38 @@
<input autocomplete="off" class="form-control" id="taginput" tabindex="3" name="tags" type="text" <% if params[:tags] || (@node && @node.tagnames) %>value="<%= params[:tags] || @node.tagnames.join(',') %>"<% else %>placeholder="balloon-mapping,gulf-coast"<% end %> data-provide="typeahead" />
</div>

<br />
Copy link
Member

Choose a reason for hiding this comment

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

Can this be presented in a partial, please?

@lalithr95
Copy link
Member Author

@jywarren I'll update code and address the comments.

@ebarry
Copy link
Member

ebarry commented Aug 15, 2016

Hi @jywarren i can sketch, can you call me at your earliest convenience?

+1 336-269-1539
@lizbarry http://twitter.com/lizbarry

On Mon, Aug 15, 2016 at 12:35 AM, Lalith Rallabhandi <
notifications@github.com> wrote:

@jywarren https://github.com/jywarren I'll update code and address the
comments.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#650 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAJ2nyg4AphJmJsB1bU-UmtKx7s2QW6qks5qf-yGgaJpZM4JeY1Z
.

@ebarry
Copy link
Member

ebarry commented Aug 15, 2016

Hey Lalith,
How are you? Hope you are well! It's quite hot here in Brooklyn -- i'm hoping that Canada is having wonderful weather?

I'm gonna repost my gif from above and talk through some of the little details.
locationinterface

  • when dragging the map, the highlighted box should change while you drag. The crosshairs stay fixed in the middle.
  • The highlighted box should be red (or hot pink, something bright).
  • We must provide two ways -- text and map -- to input as well as to display location. That's why my gif shows "This is what others will see" for both text and map display, for either way of inputting location. When we are displaying content, we may choose to ether print the location in text, or to show the entire map. We still need to think through when we'd choose which format.

What do you think about this?

@lalithr95
Copy link
Member Author

@jywarren Can you give another shot for review ? I updated content and add draggable location picker. I'm using circle instead of square. I see circle or marker is being used widely for dragging location to a place, even with google maps. It works like charm.

@jywarren
Copy link
Member

Can you post a screenshot so we can see? Thanks.

@lalithr95
Copy link
Member Author

Both marker and circle are draggable until, note change.
6
7
8
9

@lalithr95
Copy link
Member Author

Added Locationfilter plugin which allows user to select a location with square marked and also can be adjusted by drag and drop.
10
11

@lalithr95
Copy link
Member Author

@jywarren Can you have a look with this new code ?

@jywarren
Copy link
Member

Hi, Lalith - it seems like if you get the grid-square selection working for profile locations, the same interface could be used here, don't you think? The selection interface here is all right, but would require storing two points (corners) rather than a truncated lat/lon pair, isn't that right?

Why don't we focus on the profile location interface for now, and once that's running really well, we can re-apply the interface work from it to this as well.

Also note that when using external libraries, it's best to add them as an entry in /bower.json so that they are pulled in but not version tracked. See the existing bower.json file for examples of how to reference a specific release from a Github URL (although if https://bower.io lists an available library, you can just reference it by its name and version # instead of a URL) and how to include it in a given javascript file here: https://github.com/publiclab/plots2/blob/master/app/assets/javascripts/application.js

@ryzokuken
Copy link
Member

Hello! You must've noticed that the unit tests failed unexpectedly on your PR. This was due to some issue in the Travis CI testing configuration which was fixed by @jywarren later tonight. In order to make them work again, please rebase your work on top of the current master and then try pushing to your remote branch. Do this by:

$ git fetch upstream
$ git checkout master
$ git merge upstream/master
$ git checkout <my-branch>
$ git rebase master
$ git push origin <my-branch>

Thanks for bearing with us,
Ujjwal

@jywarren
Copy link
Member

OK, this is now moving to https://github.com/publiclab/leaflet-blurred-location! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants