-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Create /map/ route for wiki pages #7061
Conversation
Based on feedback in #6993 I've changed the routes and URL hash pattern Currently there is no way to specify active layers, that can be added on to the hash if it is needed! |
Looks great!!! |
|
Hey @nstjean , i think we will be solving all these things here, right? |
@sagarpreet-chadha I've marked off the 3 that are taken care of with my current PRs! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really awesome. Ready to merge?!!?
8caa23a
to
6acea52
Compare
Codecov Report
@@ Coverage Diff @@
## master #7061 +/- ##
==========================================
+ Coverage 80.83% 80.98% +0.15%
==========================================
Files 97 97
Lines 5572 5585 +13
==========================================
+ Hits 4504 4523 +19
+ Misses 1068 1062 -6
|
@jywarren This is all set now! |
Ah, i'm sorry, i wanted to add one more small request - could we left-align the button? Apologies! Then this is perfect! |
Yes of course! I'll get that moved. |
6acea52
to
aea3216
Compare
Now the codecov test is failing. Do I need to do something different? |
Hmm, I don't think so - i want to see if we can modify the thresholds; i think it's saying that it doesn't like that the new code is not tested to at least the percentage coverage of the overall repository. We could add a functional test or a system test to address this, maybe? We're just getting used to the new CodeCov checks, sorry! |
Hmmm ok. I was going to add some tests for this anyways! |
Ok I added map_test.rb to system tests to test out various map options and display. It should cover the map#wiki controller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are awesome!!!
const bounds = new L.LatLngBounds( | ||
new L.LatLng(84.67351257, -172.96875), | ||
new L.LatLng(-54.36775852, 178.59375) | ||
); | ||
|
||
var user_lat = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes awesome i wanted this to be removed from here and not the URLHash logic! Thanks!
@@ -0,0 +1,53 @@ | |||
function urlMapHash() { | |||
// This is based off of jywarren's urlhash, made specific to our map hash needs | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey this is cool, can you tell what specific changes you did to match plots2? Is the URL different now? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the format to be #zoom/lat/lon
. I really like having a hash function available to get and set the values!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, have the layer names been removed from the hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently yes they aren't in the hash. It had been set up to use the route for the layer names, which had to be changed. Do you need them added back in? It's easy enough to add them at the end of the hash, if that works for @jywarren ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's fine for now. I was trying to figure out an issue when I noticed we don't have them anymore. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had them to track active layers. In LEL it is used in the embed to preserve the state of the layers. I don't remember if this was the case in plots2? @sagarpreet-chadha, could you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey,
So i made embed feature to take array of layers as input, and show them on map by default (@nstjean ...we will use this in /people map in plots2). This feature is unrelated to url hash AFAIK.
If we will require layer names in future, it should be pretty easy, right? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Got it. To correct what I said earlier, it wasn't in the hash but before it. Thanks, @sagarpreet-chadha!
Wow, I don't know what was changed with the codecov, but now my patch went from 70% before to 15% now! |
Ah, i think it gets logged gradually, so by the time the Travis run finishes it may be much higher. I wish it notified about that! Let's see! |
I took a look and ran it in my local dev to try out some different numbers. I believe that page may be broken. Take a look: https://publiclab.org/contributors/general plots2/test/functional/tag_controller_test.rb Lines 372 to 385 in df0b944
What do you want me to do? Fixing this page could take a while. |
6dd959f
to
498d826
Compare
Hooray!! @jywarren this passed the tests! I think it's ready for merge! |
Awesome!! we can test it at stable.publiclab.org now. |
Yay!!! Oh this is exciting to see, haha. |
* add route for map/wiki/:id and display wiki location * add error messages for no location, no wiki * fix codeclimate error * display url hash on page load * Add buttons on wiki pages to link to map or prompt location entry * add redirect from /maps/ to /map/ * change url hash format to #zoom/lat/lon * fix codeclimate error * change button on wiki and page to new hash format * change PublicPagesTest test from /maps to /map * clean up code * remove center tag and unnecessary javascript * add map tests to system tests * include 0 in testing numbers * increase capybara default max wait time * change test tag contributors to a fixed tag name to stop error * remove two tests to check if it passes on server * add map controller tests * attempt to fix travis error * comment out all system map tests * format assertion in show map by hash location in a different way * hide show map by hash location, show correct url for wiki map * Increase wait time for map system tests * update yarn.lock * set max wait time to 90
* add route for map/wiki/:id and display wiki location * add error messages for no location, no wiki * fix codeclimate error * display url hash on page load * Add buttons on wiki pages to link to map or prompt location entry * add redirect from /maps/ to /map/ * change url hash format to #zoom/lat/lon * fix codeclimate error * change button on wiki and page to new hash format * change PublicPagesTest test from /maps to /map * clean up code * remove center tag and unnecessary javascript * add map tests to system tests * include 0 in testing numbers * increase capybara default max wait time * change test tag contributors to a fixed tag name to stop error * remove two tests to check if it passes on server * add map controller tests * attempt to fix travis error * comment out all system map tests * format assertion in show map by hash location in a different way * hide show map by hash location, show correct url for wiki map * Increase wait time for map system tests * update yarn.lock * set max wait time to 90
Fixes #7060 (<=== Add issue number here)
Created a route /map/wiki/:wikislug - I couldn't use /map/:wikislug because we already had a route with that pattern.
If Wiki exists then it is checking for location info to display: