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

Search page #3357

Merged
merged 11 commits into from
Sep 21, 2018
Merged

Search page #3357

merged 11 commits into from
Sep 21, 2018

Conversation

stefannibrasil
Copy link

@stefannibrasil stefannibrasil commented Sep 19, 2018

fixes #987
fixes #2421
@jywarren , sorry for this big PR again, breaking into small Pull Request is a skill itself! But we believe but if you read this first it will help you with the review! If you could switch to our branch it would be great (or maybe trying on the unstable?), because the iteration on the /search and on the typeahead would be good to be reviewed live.

  1. We decoupled DocResult from SearchService. Everyone now can use the SearchService endpoints by calling ExecuteSearch.by(:endpoint, search_criteria). We moved the DocResult packaging to the api/search.rb Class since this is the one who deals with the API. That's gonna be documented next week :)

  2. The confusing thing that was happening that the pages and notes were due to the old queries on the SearchService. Right now, the typeahead results and the /search results are not using the same methods, for exampe. For now, we created a new endpoint for the wikis (old 'pages'). Maybe we can unify them all under a general search_nodes in the future, but that's something for later.

  3. The new /search page is working and we can filter the Notes and Wikis results from sort_by options. The navbar search and the form on /search use now the same methods. Before the /search was only returning Notes... now the results are consistent. It includes now the static pages as well.

We just need to finish some small UI details but we thought it would be better to open this PR to ask for your review and open a new one tomorrow maybe.

As Camilla mentioned before, this is our last work on the coding. Along the way, we wrote some docs and we plan to create detailed issues of the tasks that we couldn't finish.

@plotsbot
Copy link
Collaborator

plotsbot commented Sep 19, 2018

2 Messages
📖 @stefannibrasil 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.
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.

Generated by 🚫 Danger

@milaaraujo
Copy link
Collaborator

1

@milaaraujo
Copy link
Collaborator

2

@jywarren
Copy link
Member

This looks amazing!!! I've downloaded it and am compiling now.

@jywarren
Copy link
Member

i'm pushing it to unstable too in case that installs faster than my laptop (its having me upgrade and compile some recently updated gems)

@jywarren
Copy link
Member

Ok I like this a lot - is it running on unstable now as you expect?

Is there a possibility of an "all" category, or is that too complicated?

I think we can probably merge this now if you think it's ready!

@stefannibrasil
Copy link
Author

stefannibrasil commented Sep 20, 2018

I just pushed some small tweaks, @jeff. Sorry, can you try again?

The reason it doesn't have the 'all' pages right now is that we don't have any partial to use to show the all type of results on the search view... unless we use just a table for now. We can do this later today, no problem! That was on the list :D

about the unstable, I'm seeing the old search page here, on yours as well? 🤔

@stefannibrasil
Copy link
Author

stefannibrasil commented Sep 20, 2018

and it's also not showing the 'Show all' option on the typeahead... it doesn't look like it's our code there on unstable xD

@jywarren
Copy link
Member

Ah, so I see https://unstable.publiclab.org/search/balloon doesn't exist anymore. Maybe we should redirect it to https://unstable.publiclab.org/search/notes/balloon until we think through an "all" search? Thanks!

@jywarren
Copy link
Member

ah i think somebody overwrote unstable... maybe check in the chatroom?

@jywarren
Copy link
Member

Actually i realized that there may not be a quick way to integrate users and notes... maybe the default "all" view would be just notes + pages + questions... OR we should just default to notes.

@jywarren
Copy link
Member

sorry, in meetings today but trying to respond between! Having staff retreat -- staff say hello!

@jywarren
Copy link
Member

I was able to test it on unstable before it seemed to have restarted, and saw the new interface with buttons for different types. It was great.

@jywarren
Copy link
Member

OK, looks like it's running now:

image

If we can have /search/____ redirect to /search/notes/_____ that would make it ready to merge in my opinion! This is awesome. 👍 🎉 🎈

@jywarren
Copy link
Member

Hmm, i do see an error - when i type the phrase (with quotation marks): 'open call' then some of the buttons don't work:

https://unstable.publiclab.org/search/notes/'open%20call'?order=natural&type=boolean

the Questions and Tags buttons don't seem to work on that page; it just goes to https://unstable.publiclab.org/search/questions/

However, without quotations, like searching for balloon -- https://unstable.publiclab.org/search/balloon/ -- it works and i see a proper page render at https://unstable.publiclab.org/search/questions/balloon/ (although it says no results as it should).

@jywarren
Copy link
Member

Hmm, are the per-type searches not working in this version of the code?

image

Is there a reason it's no longer separating the typeahead results into different types? Will this be re-instated once it's merged into the master branch, over your last PR?

@stefannibrasil
Copy link
Author

hi @jeff, try again now!! the only problem is that by setting the typeahead items to 15 we can't see the 'show all' button. I think 10 it's better! we can discuss more the details tomorrow, see you!

Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

:-)

Follow up issues maybe

  1. Separate the search type button group from ordering button group? (Put them in button groups)
  2. Asterisks?

config/routes.rb Outdated
get "search/wikis/:query", :to => "search#wikis"
get "search/profiles/:query", :to => "search#profiles"
get "search/questions/:query", :to => "search#questions"
get "search/places/:query", :to => "search#places"
get "search/tags/:query", :to => "search#tags"
get "search/", :to => "search#new"
get "search/:query", :to => "search#notes"
Copy link
Member

Choose a reason for hiding this comment

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

Great!

@jywarren jywarren merged commit a128622 into publiclab:master Sep 21, 2018
@ghost ghost removed the review-me label Sep 21, 2018
SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
* Decouple DocResult from SearchService

* Add new /search page

* Fix tests

* removing 'returns'

* Fixing notes ans wikis parameters

* Fixing notes ans wikis parameters [2]

* small tweaks

* change search/notes to search/* default and add html_safe to search/ queries

* we need to fix those search scripts

* add type to search node.search method and fix urls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants