search

CUTR-at-USF / usf-mobullity

USF Maps responsive web application
http://maps.usf.edu/
8 stars 7 forks source link

Zoom on USF campus #65

Closed jailby closed 9 years ago

jailby commented 9 years ago

If users don't share their location the map will give a super zoomed out view of West Florida. Screenshot from Firefox: image

When users don't share their location the map should be zoomed on the USF campus. Then, users can manually adjust to their preferences.

Related to: https://github.com/CUTR-at-USF/usf-mobullity/issues/45

jailby commented 9 years ago

@jmfield2 do you have an idea of which file handle the zooming part of the code?

jmfield2 commented 9 years ago

I think the default zoom is in js config and it is executed somewhere in js otp core webapp.js On Aug 31, 2015 9:49 AM, "jailby" notifications@github.com wrote:

@jmfield2 https://github.com/jmfield2 do you have an idea of which file handle the zooming part of the code?

— Reply to this email directly or view it on GitHub https://github.com/CUTR-at-USF/usf-mobullity/issues/65#issuecomment-136376863 .

jailby commented 9 years ago

I made some modification that made the map to be more zoomed-in on USF campus.

When changing the attribute "zoomToFitResults" to true the default location is now centered on USF campus.

I haven't found yet how to modify the default parameters. I change the default location but nothing appear to be changing. I need to find where exactly it is executed. I didn't found it in webapp.js but I'm still looking.

The attribute "initZoom" should be the one influencing this behavior but nothing happen when I modify it. Thus, the execution may overwrite it.

jmfield2 commented 9 years ago

Sorry, it isn't in Webapp.js it's in otp core Map.js (This is also where the L.control for the my location button will probably need to go for #70)

Also, it seems the reason why initzoom isn't working is because of: https://github.com/CUTR-at-USF/usf-mobullity/blob/mobullityrebase/src/client/js/otp/core/Map.js#L103

Basically, Map.js gets the /metadata API from OTP to find the full map boundary for the loaded map data. Currently this is the full metro area - which is why the map opens up by default w/o gps to the greater tampa area, and overriding both the initial zoom and center values from config.js.

I imagine the proper fix will be to remove this fitBounds code - instead skipping right to attempting the initial geolocation but it should probably include the map boundary ajax call so that it can validate the gps coordinates in onLocationFound below in case students use the app off campus.

Then, it will only center on USF if: (recapping and extending previous discussion on this topic) 1) The user denies GPS, 2) The user is on campus (and it should zoom in closer, currently this is hard-coded @ 17),
3) The GPS coordinates are outside of the map boundaries (aka not in the tampa area), 4) The GPS coordinates had an accuracy of "22000" which is a city-center approximation for wired connections .. This may be annoying for some wired connections outside of campus, but probably not a big deal.

Note for 2), see: https://github.com/CUTR-at-USF/usf-mobullity/blob/mobullityrebase/src/client/js/otp/core/Map.js#L155

I hope all of that helps :)

jailby commented 9 years ago

Yeah I was on this file yesterday but since I had to go I wanted to left an update.

Thus, to make sure, the only line to delete is from 103 to 106 right? The rest of the function creates the boundaries. @barbeau Should I delete these lines or comment and explain them?

Then; 1) Should be fixed by the deletion of the line 103 to 107, and would only depend on the "initzoom" attribute. 2) If the the user gps coord are on campus it zoom in on this gps coord right? and it's the desired behavior. (I'm not sure if you meant that there is something to do about it or just explaining) 3) and 4) are working, right? Nothing to do about it except if 4) become an issue.

Let me know if you have any additional comment about anything.

jmfield2 commented 9 years ago

I think that's probably easiest and then do a PR On Sep 2, 2015 9:40 AM, "jailby" notifications@github.com wrote:

A fix has been provided in the issue https://github.com/opentripplanner/OpenTripPlanner/issues/2111#issuecomment-137046621 of the Official OTP which is here https://github.com/buma/OpenTripPlanner-Maribor/commit/664fbfb3cd06dc6245bbe8d2cb1dba83e2692476 .

What is the procedure to apply the change? Should I just modify the file "WalkableAreaBuilder.java" to mirror the changes?

— Reply to this email directly or view it on GitHub https://github.com/CUTR-at-USF/usf-mobullity/issues/65#issuecomment-137085092 .

jailby commented 9 years ago

I actually tried it and it makes the map not working at all.

I was investigating on why it would do so. If you have an idea of why let me know.

jmfield2 commented 9 years ago

I think that's all that's needed yes

2) I was explaining mostly - but the closer zoom might be a good idea to define in the config instead of being hardcoded as 17

For 3 and 4 yes they seem to work currently and should be fine as long and metadata result is saved like now On Sep 2, 2015 9:34 AM, "jailby" notifications@github.com wrote:

Yeah I was on this file yesterday but since I had to go I wanted to left an update.

Thus, to make sure, the only line to delete is from 103 to 106 right? The rest of the function creates the boundaries.

Then; 1) Should be fixed by the deletion of the line 103 to 107, and would only depend on the "initzoom" attribute. 2) If the the user gps coord are on campus it zoom in on this gps coord right? and it's the desired behavior. (I'm not sure if you meant that there is something to do about it or just explaining) 3) and 4) are working, right? Nothing to do about it except if 4) become an issue.

Let me know if you have any additional comment about anything.

— Reply to this email directly or view it on GitHub https://github.com/CUTR-at-USF/usf-mobullity/issues/65#issuecomment-137082258 .

jmfield2 commented 9 years ago

Lol hm can you show me a diff of the map file ? I tried actually just returning after l.lmap is called above and it seemed to work ... Maybe too much was removed or some other syntax-like issues? On Sep 2, 2015 9:58 AM, "jailby" notifications@github.com wrote:

I actually tried it and it makes the map not working at all.

I was investigating on why it would do so. If you have an idea of why let me know.

— Reply to this email directly or view it on GitHub https://github.com/CUTR-at-USF/usf-mobullity/issues/65#issuecomment-137090361 .

jailby commented 9 years ago

I just did that: / success: function(data) {
this_.lmap.fitBounds([ [data.lowerLeftLatitude, data.lowerLeftLongitude], [data.upperRightLatitude, data.upperRightLongitude] ]);
//

jmfield2 commented 9 years ago

Yup too much - try moving the /\ after success: On Sep 2, 2015 10:05 AM, "jailby" notifications@github.com wrote:

I just did that: /** success: function(data) {

this_.lmap.fitBounds([ [data.lowerLeftLatitude, data.lowerLeftLongitude], [data.upperRightLatitude, data.upperRightLongitude] ]);

/**/

— Reply to this email directly or view it on GitHub https://github.com/CUTR-at-USF/usf-mobullity/issues/65#issuecomment-137092927 .

jailby commented 9 years ago

Yeah that's not working either. What change did you make that doesn't make the map fail like me?

jailby commented 9 years ago

Actually it's working, sorry.

jailby commented 9 years ago

@barbeau the following is the view we want when a user refuse to share its location: image

It is center on this GPS coordinates: (28.061062, -82.413200) and the initZoom value is 15. Also, should I delete the lines creating the problems? I just want a confirmation before creating the PR.

barbeau commented 9 years ago

@jailby Looks good to me! Yes, please remove any dead/commented code. We have version control via Git to go back and look at old code if needed.

jailby commented 9 years ago

The code is missing a comma so the production is not working at the moment.

jailby commented 9 years ago

The view is now zoomed in on the trip planned but two issue appeared:

1) On lower resolution screen the view became too zoomed in. The solution could be to zoomed out a little more. The default zoomed in may be too much. However, this could generate a problem for high resolution screen which could end up too zoomed out. Adapt depending on the screen resolution is probably the best way to fix this problem. #74

2) When the user share his/her location the view zoom on the trip and then zoom on the user's location. This might come from the function ordering. Thus, the user location dependent function should happen before the function zooming on the trip. #45