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 default location to /post #6957

Merged
merged 9 commits into from
Jan 6, 2020

Conversation

nstjean
Copy link
Contributor

@nstjean nstjean commented Dec 12, 2019

Fixes #6946 (<=== Add issue number here)

This adds in the user's profile location as a default value for location.

  • Updated button to change to "Remove location" when it has a location. It will toggle back and forth. Location values are only saved when map is visible.
  • Added a function getPrecisionFromNum - I recommend this be placed in leaflet-blurred-location. The precision number is then used to call setZoomByPrecision which already exists.
  • Should we update the code here
    }).setView([<%= lat %>,<%= lon %>], <%= lat.to_s.length.to_i %> + 6);
    with the new calculator, since it appears to be doing a different calculation than what is used in LBL
  • PL.Editor does has lat: and lon: in the constructor, but not zoom:. I called editor.mapModule.blurredLocation.setZoomByPrecision after the map was contructed to set the zoom level. Perhaps zoom could be added to the constructor?

FireShot Capture 108 - 🎈 Public Lab_ Post - localhost
FireShot Capture 109 - 🎈 Public Lab_ Post - localhost

@nstjean
Copy link
Contributor Author

nstjean commented Dec 12, 2019

Still having that NodeShared test issue. 😞

@@ -404,6 +406,29 @@
}
});

editor.mapModule.blurredLocation.setZoomByPrecision(getPrecisionFromNum("<%= @lat %>"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome @nstjean !
But this getPrecisionFromNum function needs to be removed. I have added more details here: #6946

Briefly - the zoom level calculation needs to be inside a function in LBL.
The PL.editor will just take that zoom level as input and just set map to that zoom in map_module of PL.editor.
If zoom is present in the form of zoom:xx in plots2, let's pass that otherwise call the function of LBL to get zoom and then pass it. Thanks 😄 !!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh, ok! That makes sense!

@nstjean
Copy link
Contributor Author

nstjean commented Dec 18, 2019

  • I removed getPrecisionFromNum function, now using blurredLocation.getZoomFromCoordinates

publiclab/leaflet-blurred-location#222 must be merged first so it works!

  • This checks the user for a zoom level, if it exists it uses it on the map. If it doesn't exist it gets a zoom from blurredLocation and then saves it as a powertag zoom:xx

editor.mapModule.blurredLocation.setZoom(<%= @zoom %>);
<% else %>
let zoom = editor.mapModule.blurredLocation.getZoomFromCoordinates(<%= @lat %>, <%= @lon %>);
addTag('zoom:'+zoom, '/profile/tags/create/<%= current_user.uid %>');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we are explicitly adding tag here?
Pl.editor will automatically add zoom tag, right?
Moreover if user changes zoom level, we will have 2 zoom tags? One from here and other from PL.editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only calculates zoom in the instances that the user has a lat and lon in their profile but no zoom level saved. So it calculates a zoom and saves it to their profile using the addTag javascript function (I worked on that a few weeks ago). It does not add the tag into the map, just to their profile. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i see, the name is misleading. Very cool place to put this though :)

editor.mapModule.blurredLocation.setZoom(<%= @zoom %>);
<% else %>
let zoom = editor.mapModule.blurredLocation.getZoomFromCoordinates(<%= @lat %>, <%= @lon %>);
addTag('zoom:'+zoom, '/profile/tags/create/<%= current_user.uid %>');
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh i see, the name is misleading. Very cool place to put this though :)

@nstjean
Copy link
Contributor Author

nstjean commented Jan 1, 2020

@jywarren I think this all set but codeclimate is complaining!

@jywarren
Copy link
Member

jywarren commented Jan 2, 2020

Sorry for slow catchup here, i just resolved that. All good to merge then? Thanks!

@jywarren
Copy link
Member

jywarren commented Jan 2, 2020

Ah, we now have a couple merge conflicts to resolve. Would you mind taking a look? Thanks!

@codecov
Copy link

codecov bot commented Jan 2, 2020

Codecov Report

Merging #6957 into master will increase coverage by 0.14%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6957      +/-   ##
==========================================
+ Coverage   80.69%   80.83%   +0.14%     
==========================================
  Files          97       97              
  Lines        5568     5574       +6     
==========================================
+ Hits         4493     4506      +13     
+ Misses       1075     1068       -7
Impacted Files Coverage Δ
app/controllers/editor_controller.rb 74.41% <83.33%> (+1.44%) ⬆️
app/helpers/application_helper.rb 84.52% <0%> (+1.19%) ⬆️
app/api/srch/search.rb 68.62% <0%> (+3.92%) ⬆️
app/services/execute_search.rb 94.44% <0%> (+5.55%) ⬆️

@jywarren
Copy link
Member

jywarren commented Jan 2, 2020

Hmm, looks like this is the error, odd!

ERROR["test_posting_from_the_editor", #<Minitest::Reporters::Suite:0x00007f8ff6c707f8 @name="PostTest">, 77.13617552799997]
 test_posting_from_the_editor#PostTest (77.14s)
Capybara::ElementNotFound:         Capybara::ElementNotFound: Unable to find css ".wk-wysiwyg"
            test/system/post_test.rb:19:in `block in <class:PostTest>'

https://travis-ci.org/publiclab/plots2/jobs/631972389#L3767

@nstjean
Copy link
Contributor Author

nstjean commented Jan 6, 2020

That's weird. That class is in PublicLab.Editor - I'll try yarn upgrade and push again to see what happens.

@plotsbot
Copy link
Collaborator

plotsbot commented Jan 6, 2020

1 Warning
⚠️ There was an error with Danger bot’s Junit parsing: No JUnit file was found at output.xml
2 Messages
📖 @nstjean Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 #
Screenshots 📸 (click to expand)

6957-test_questions.png

6957-test_embeddable_grids.png

6957-test_signup.png

6957-test_viewing_the_settings_page.png

6957-test_tag_by_author_page.png

6957-test_wiki_page_with_inline_grids.png

6957-test_stats.png

6957-test_viewing_the_dashboard.png

6957-test_searching_an_item_from_the_homepage.png

6957-test_signup_modal_form_validation.png

6957-test_questions_shadow.png

6957-test_login_modal.png

6957-test_profile_page.png

6957-test_comments.png

6957-test_tags.png

6957-test_signup_modal.png

6957-test_wiki.png

6957-test_methods.png

6957-test_tag_page.png

6957-test_blog_page_with_location_modal.png

6957-test_tag_wildcard.png

6957-test_signup_modal_disabled_submit_button_on_empty_username.png

6957-test_embeddable_thumbnail_grids.png

6957-test_front_page_with_navbar_search_autocomplete.png

6957-test_login.png

6957-test_viewing_the_dropdown_menu.png

6957-test_viewing_question_post.png

6957-test_mobile_displays.png

6957-test_simple-data-grapher_powertag.png

6957-test_front.png

6957-test_question_page.png

6957-test_tag_contributors_page.png

6957-test_blog.png

6957-test_people.png

6957-test_wiki_revisions.png

Learn about automated screenshots

Generated by 🚫 Danger

@nstjean
Copy link
Contributor Author

nstjean commented Jan 6, 2020

@jywarren Fixed it! Should be all set to merge!

@jywarren jywarren merged commit f61b91e into publiclab:master Jan 6, 2020
@sagarpreet-chadha
Copy link
Contributor

Awesome!

Tlazypanda pushed a commit to Tlazypanda/plots2 that referenced this pull request Jan 14, 2020
* update yarn

* add default location to post form

* keep default lat/lon zoom level by calculating precision

* display zoom or calculate and save zoom

* use LBL's getZoomFromCoordinates for zoom and style location button

* fix codeclimate error with parentheses

* fix codeclimate error

* make map slider in modal update when map is zoomed

* fix merge
vinitshahdeo pushed a commit to vinitshahdeo/plots2 that referenced this pull request Feb 1, 2020
* update yarn

* add default location to post form

* keep default lat/lon zoom level by calculating precision

* display zoom or calculate and save zoom

* use LBL's getZoomFromCoordinates for zoom and style location button

* fix codeclimate error with parentheses

* fix codeclimate error

* make map slider in modal update when map is zoomed

* fix merge
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.

Geographic: Set a default location of user's profile location on /post/
4 participants