Router_mixin implemented for SMap router functionality and state control #1326

linzjax · 2017-03-21T18:55:34Z

NOTE: NOT BEING MERGED INTO MASTER. BEING MERGED INTO map-spa-router

Proposed changes in this pull request

Added router_mixin for all functions that update the state of the current page.
Added hooks to the HTML to allow ajax interception of form submission
Handles permission errors are redirects for anonymous and non-permissioned users.
Added current_active_tab to the state to allow for automatic redirecting to last active tab.
Selected location is now automatically centered on if reached through URL. popup updated to show currently viewing
Added mobile compatibility for displaying and hiding project detail panel
Sidebar tabs are updated when switching from map to overview
Addresses SMAP: Router should handle permission errors #1207 , and SMAP: Redirecting to specific location tab #1206 (though this could use fine-tuning now that it's an option. I.E., If I come from the resource page to a specific location, would I expect to see the resources tab open?).

When should this PR be merged

Whenever

Risks

This is a lot of code, and since we don't have a JS test kit, I did my best but it could definitely use new eyes.

Follow-up actions

Once we pull this full feature in we'll need to do a thorough comb through to make sure all the new stuff from master hasn't been messed up. The rebase happened, but because there were so many changes, if anything is/was changed in the HTML, it might have been botched.

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly? It should have the migration label if a new migration is added.
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

** Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.**
** Are all UI and API inputs run through forms or serializers?**
** Are all external inputs validated and sanitized appropriately?**
** Does all branching logic have a default case?**
** Does this solution handle outliers and edge cases gracefully?**
** Are all external communications secured and restricted to SSL?**

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

…1.0 (#1077) * Single page map scaffolding view and html. Upgrade single page to Leaflet 1.0 - ProjectMap will server as the main view for all project pages - Side pannel can be displayed or hidden by switching between detail-hidden and detail-display - Upgrade JUST project_map.html to Leaflet 1.0. Since other pages will be absorbed by this one, I am putting off the full upgrade until the other views have been reworked. - Currently only standard tests for ProjectMapView. Will add more as more functionality is added. - Removed L.Deflate temporarily. Running into issues with upgrade to leaflet 1.0 * ripped out django-leaflet. set up js/smap/

* implemented tile->lat/long converter adding L.Deflate back added L.TileLayer.GeoJSON and updated to be compatable with Leaflet 1.0 Currently only loaded spatial resources. Should update to drop SUs when out of view * Lazy tile loading

* Add new L.Deflate, MarkerCluster code; update dashboard map * Make project maps deflatable

- moved default views and urls to async folders - refactored everything to play nicely with django-leaflet

…the current page. Added hooks to the html to allow ajax interception of form submission Handles permission errors are redirects for anonymous and non-permissioned users. added current_active_tab to state to allow for automatic redirecting to last active tab. selected location is now automatically centered on if reached through url. popup updated to show currently viewing Added mobile compatability for displaying and hiding project detail panel Sidebar tabs are updated when switching from map to overview Added onclick to td in location detail resource table

linzjax · 2017-03-21T19:07:50Z

oliverroick

Hey, it's mostly ok, just have a few comments.

Editing locations doesn't submit the form; I guess Brian is working on that?!
When I delete a location, would there be a way to keep the map position and not move it to the project extent? I can see cases, where someone wants to edit a bunch of locations in a neighbourhood; having to back to that area every time you delete something can be really annoying. Same with starting to add a location and then canceling. (In fact, there's research on map usability that suggest to never automatically move the map, unless it's an explicit user action).
Once I add a resource to one location and then open the details of another location, it opens that location in the resources tab. It's the same when I add a relationship to one locations, all other location open the relationship tab by default.
Accessing a location via its URL sometimes doesn't zoom to the location on the map. I tested this using a big project; could this be released to the location geometry not yet being loaded onto the map?
When I go to a resource detail page and then use the browser's back button it doesn't open the resource tab of the location. That's probably because the state is deleted, so maybe there should be a way to add this to the URL to preserve this.

Let me know when you want to go on a call to discuss....

oliverroick · 2017-03-24T11:22:44Z

cadasta/core/static/js/smap/router.js

-        route.eventHook(view_url);
+    var el = document.getElementById(rm.getRouteElement(route.el));
+    $.get(async_url, function(response){
+      if (response.includes("alert-warning")) {


I'm not sure if I like that. Say we include some warning with the same class name in the side, you would always redirect to the overview tab.

Would there be a way to make this more fail-safe. For example, by adding custom headers to the response that we can evaluate on client-side?

oliverroick · 2017-03-24T11:34:14Z

cadasta/templates/organization/project_map.html

 {% load leaflet_tags %}

 {% block extra_head %}
 {% leaflet_css plugins="groupedlayercontrol,markercluster" %}
-<link rel="stylesheet" href="https://unpkg.com/[email protected]/dist/leaflet.css" />
-<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/leaflet-geocoder-mapzen/1.7.1/leaflet-geocoder-mapzen.css">
+<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/leaflet-geocoder-mapzen/1.4.0/leaflet-geocoder-mapzen.css">


Is there a reason not to keep the newer version?

I was running into issues with the geocoder's responsiveness, but I think all of that's fixed on master which I haven't rebased to yet

Ah right, I think master is on 1.8.x or so. That should be ok then.

oliverroick · 2017-03-24T11:34:29Z

cadasta/templates/organization/project_map.html

-<script src="{% static 'js/smap/router.js' %}"></script>
-<script src="{% static 'js/smap/map.js' %}"></script>
+<script type="text/javascript" src="{% static 'buckets/js/script.js' %}"></script>
+<script src="https://cdnjs.cloudflare.com/ajax/libs/leaflet-geocoder-mapzen/1.4.0/leaflet-geocoder-mapzen.js"></script>


Is there a reason not to keep the newer version?

linzjax · 2017-03-24T18:31:53Z

Editing locations doesn't submit the form; I guess Brian is working on that?!

Yeah, I was holding off on anything involving the geometries for the time being.

When I delete a location, would there be a way to keep the map position and not move it to the project extent? I can see cases, where someone wants to edit a bunch of locations in a neighborhood; having to back to that area every time you delete something can be really annoying. Same with starting to add a location and then canceling. (In fact, there's research on map usability that suggests to never automatically move the map, unless it's an explicit user action).

This plays into what I'm working on with the URL map position storage. It would be nice to have a list of expected map behavior for each view/user path.

Once I add a resource to one location and then open the details of another location, it opens that location in the resources tab. It's the same when I add a relationship to one location, all other locations open the relationship tab by default.

Dangit thought I fixed that.. will work on that now

Accessing a location via its URL sometimes doesn't zoom to the location on the map. I tested this using a big project; could this be released to the location geometry not yet being loaded onto the map?

Yeah so if the location isn't in the project extent or in the initial screen view, it's not loaded by the tile loader, and that's currently where I'm getting the bounds from. The URL coords will help with that, but I can see situations where people get to a location detail without the coords already attached. I'm open to suggestions on this one...

When I go to a resource detail page and then use the browser's back button it doesn't open the resource tab of the location. That's probably because the state is deleted, so maybe there should be a way to add this to the URL to preserve this.

Hmm.. I'll have to think about this one...

I'm not sure if I like that. Say we include some warning with the same class name in the side, you would always redirect to the overview tab.

Would there be a way to make this more fail-safe. For example, by adding custom headers to the response that we can evaluate on client-side?

I figured out a better was to do this using headers like you suggested. Fixing it now.

I think a call would definitely help if you're available early next week.

linzjax and others added 6 commits March 17, 2017 10:13

Implement Vector Tiling for SMAP (#1099)

ba40226

* implemented tile->lat/long converter adding L.Deflate back added L.TileLayer.GeoJSON and updated to be compatable with Leaflet 1.0 Currently only loaded spatial resources. Should update to drop SUs when out of view * Lazy tile loading

Make dashboard and project maps deflatable (#1133)

e2f657f

* Add new L.Deflate, MarkerCluster code; update dashboard map * Make project maps deflatable

Fix #1124: Integrate cadasta fork of django-leaflet (#1152)

05af3fb

Implemented minimal router for updating map view detail pannel.

dec2a37

- moved default views and urls to async folders - refactored everything to play nicely with django-leaflet

oliverroick reviewed Mar 24, 2017

View reviewed changes

Added custom headers to form errors, permission errors, and redirects

2233293

linzjax force-pushed the feature/map-spa-router branch from dec2a37 to c459c8e Compare April 6, 2017 16:00

linzjax closed this Apr 11, 2017

linzjax deleted the feature/map-spa-router-forms branch April 21, 2017 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Router_mixin implemented for SMap router functionality and state control #1326

Router_mixin implemented for SMap router functionality and state control #1326

linzjax commented Mar 21, 2017 •

edited

Loading

linzjax commented Mar 21, 2017 •

edited

Loading

oliverroick left a comment

oliverroick Mar 24, 2017

oliverroick Mar 24, 2017

linzjax Mar 24, 2017

oliverroick Mar 24, 2017

oliverroick Mar 24, 2017

linzjax commented Mar 24, 2017

Router_mixin implemented for SMap router functionality and state control #1326

Router_mixin implemented for SMap router functionality and state control #1326

Conversation

linzjax commented Mar 21, 2017 • edited Loading

Proposed changes in this pull request

When should this PR be merged

Risks

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

linzjax commented Mar 21, 2017 • edited Loading

oliverroick left a comment

Choose a reason for hiding this comment

oliverroick Mar 24, 2017

Choose a reason for hiding this comment

oliverroick Mar 24, 2017

Choose a reason for hiding this comment

linzjax Mar 24, 2017

Choose a reason for hiding this comment

oliverroick Mar 24, 2017

Choose a reason for hiding this comment

oliverroick Mar 24, 2017

Choose a reason for hiding this comment

linzjax commented Mar 24, 2017

linzjax commented Mar 21, 2017 •

edited

Loading

linzjax commented Mar 21, 2017 •

edited

Loading