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

Sitewide Search #71

Closed
wants to merge 5 commits into from
Closed

Sitewide Search #71

wants to merge 5 commits into from

Conversation

seejee
Copy link

@seejee seejee commented Feb 19, 2012

@jordanbyron The whole issue #12 sitewide search feature is not yet complete. I haven't implemented the front-end styling yet, but I wanted you to take a look to make sure that it looks ok from the controller down to the db.

I waffled over whether to just shove a search method in the ArticlesController or make a new SearchController. I went with the latter, which forced me to move a few partials from views/articles into views/shared.

Anyway, I just wanted to give you a sneak peek while I worked on the UI bits. I look forward to the feedback!

@@ -7,6 +7,6 @@
Copy link
Member

Choose a reason for hiding this comment

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

@geihsler could I ask the reason why do you decide to move it into shared?

Copy link
Author

Choose a reason for hiding this comment

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

Well, I moved it to shared since it's being used by two different controllers. I wasn't sure if it was idiomatic to have the SearchController reach into the Articles views and use them, but I'm happy to move the articles partials back to their original location.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure view should depend on what controllers that call to render the view. Because what I think Rails follow convention to has a partial of an object in collection view itself, and that's why in Rails introduces a shortcut to render collection by just call render @articles. @jordanbyron, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@geihsler @samnang I think it's OK to reach into article views for article specific partials. The shared folder really exists for partials which don't have a home and are truly "shared" with every page like the footer and navigation sections.

Oh a related note, I don't know if displaying the full article partial in the search results is the best option. Especially when we start to display articles, people, and activities. However for right now just use the articles partial until we start pulling in more records into the search page.

@seejee
Copy link
Author

seejee commented Feb 22, 2012

Ok, I think I'm done messing around with the search box styling. Please take a look as I believe a very rudimentary search for articles is fully working. I also move the partials back to their original location.

Future improvements:

  1. Include activities and people in search results
  2. Style the search results differently to differentiate them from the regular articles

Anything else?

I just want to caveat all of this work by saying that I have don't much rails (or web) work, so I'm very open to areas of improvement. Thanks!

@seejee seejee closed this Feb 20, 2020
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.

3 participants