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.

Sign up for GitHub

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

Jump to bottom

Search maps by a location #924

Merged
merged 10 commits into from
Aug 27, 2019
Merged

Search maps by a location #924

merged 10 commits into from
Aug 27, 2019

Conversation

divyabaid16
Copy link
Contributor

@divyabaid16 divyabaid16 commented Aug 11, 2019 •
edited by grvsachdeva
Loading

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

Peek 2019-08-11 13-36

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/mapknitter-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

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

Thanks!

codeclimate[bot]
codeclimate bot reviewed Aug 11, 2019
View reviewed changes
app/helpers/front_ui_helper.rb Outdated
@@ -10,4 +10,8 @@ def anonymous(maps)
def featured(maps)
maps.featured
end

def location(maps,loc)
maps.location(loc)
Copy link

Choose a reason for hiding this comment

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

Use 2 (not 1) spaces for indentation.

app/helpers/front_ui_helper.rb Outdated
@@ -10,4 +10,8 @@ def anonymous(maps)
def featured(maps)
maps.featured
end

def location(maps,loc)
Copy link

Choose a reason for hiding this comment

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

Space missing after comma.

app/helpers/front_ui_helper.rb Outdated
@@ -10,4 +10,8 @@ def anonymous(maps)
def featured(maps)
maps.featured
end

def location(maps,loc)
maps.location(loc)
Copy link

Choose a reason for hiding this comment

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

Tab detected.

@codeclimate
Copy link

codeclimate bot commented Aug 11, 2019

Code Climate has analyzed commit e31d0f07 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 3

View more on Code Climate.

@codecov
Copy link

codecov bot commented Aug 11, 2019 •
edited
Loading

Codecov Report

Merging #924 into main will increase coverage by 0.46%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #924      +/-   ##
==========================================
+ Coverage   72.78%   73.25%   +0.46%     
==========================================
  Files          37       37              
  Lines        1356     1361       +5     
==========================================
+ Hits          987      997      +10     
+ Misses        369      364       -5
Impacted Files Coverage Δ
app/controllers/front_ui_controller.rb 78.78% <100%> (+3.78%) ⬆️
lib/exporter.rb 94.17% <0%> (+2.24%) ⬆️

@divyabaid16
Copy link
Contributor Author

Hi, @gauravano I guess I have some routing problem here.
I want to pass the js variable to the controller so that I can get a ruby variable and from that get the div to load.
The problem is that I am not able to access the value of the variable @loc from the controller. Maybe there is some issue with the routes. Can you please look into this?
Thanks!

@divyabaid16
Copy link
Contributor Author

Implementing the map is still left. This is a WIP PR.

@grvsachdeva
Copy link
Member

checking

@grvsachdeva
Copy link
Member

Why are you loading using AJAX in view. Use link_to as button. From there, make request, and from controller render js -- https://coderwall.com/p/kqb3xq/rails-4-how-to-partials-ajax-dead-easy

@grvsachdeva
Copy link
Member

grvsachdeva commented Aug 12, 2019 •
edited
Loading

Otherwise, you can pass the location using render :json => { :success => "success", :loc => @loc } in controller. This will trigger the success callback and also give you the location in data arg.

But, prefer to implement using #924 (comment)

@grvsachdeva
Copy link
Member

grvsachdeva commented Aug 12, 2019 •
edited
Loading

Although, this feature is a good addition but I doubt if this closes #903. As #903 is about showing all images for a location.

@divyabaid16
Copy link
Contributor Author

Ya exactly. After doing this I realized that this is already present in the search bar of the header.
I will just send the PR showing the images on the map.

@divyabaid16
Copy link
Contributor Author

it's done @gauravano

@grvsachdeva grvsachdeva self-requested a review August 16, 2019 21:53
@grvsachdeva
Copy link
Member

Great, will review in the morning. Also, there's a merge conflict, please resolve that.

@divyabaid16
Copy link
Contributor Author

I have resolved the merge conflict @gauravano
Thanks!

@divyabaid16
Copy link
Contributor Author

Done @gauravano

grvsachdeva
grvsachdeva reviewed Aug 17, 2019
View reviewed changes
app/models/map.rb Outdated Show resolved Hide resolved
@grvsachdeva
Copy link
Member

Hi @divyabaid16, then let's wait for that error to be resolved. I will also see if I can help with the Recaptcha error. Also, please write a test for the method. Thank you!

@divyabaid16
Copy link
Contributor Author

@gauravano done!

@jywarren jywarren merged commit 1ac63ab into publiclab:main Aug 27, 2019
chen-robert pushed a commit to chen-robert/mapknitter that referenced this pull request Dec 5, 2019
@divyabaid16 @chen-robert
Search maps by a location (publiclab#924)
7316b63 
* Search maps by location

* mend

* Search by location -2

* Error fixed

* routes updated

* Naming changed

* Changes fixed

* Like operation fixed

* Error 500 solved

* Test added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grvsachdeva grvsachdeva grvsachdeva left review comments

@codeclimate CodeClimate codeclimate left review comments

@jywarren jywarren jywarren approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@divyabaid16 @grvsachdeva @jywarren