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

Conversation

Projects
None yet
4 participants
@stefannibrasil
Collaborator

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

This comment has been minimized.

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 added some commits Sep 20, 2018

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 20, 2018

1

@milaaraujo

This comment has been minimized.

Collaborator

milaaraujo commented Sep 20, 2018

2

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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 stefannibrasil force-pushed the milaaraujo:search-page branch from e2be09d to e7c19fd Sep 20, 2018

@stefannibrasil

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

@jywarren

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Contributor

jywarren commented Sep 20, 2018

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

This comment has been minimized.

Collaborator

stefannibrasil commented Sep 21, 2018

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!

@jywarren

:-)

Follow up issues maybe

  1. Separate the search type button group from ordering button group? (Put them in button groups)
  2. Asterisks?
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"

This comment has been minimized.

@jywarren

jywarren Sep 21, 2018

Contributor

Great!

@jywarren jywarren merged commit a128622 into publiclab:master Sep 21, 2018

6 checks passed

ci/gitlab/unstable Pipeline passed on GitLab
Details
codeclimate Approved by Jeffrey Warren.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at ?%
Details
danger/danger All green. Good on 'ya.
Details

@wafflebot wafflebot bot removed the review-me label Sep 21, 2018

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