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

569 search incidents only #592

Merged
merged 12 commits into from Feb 15, 2018
Merged

569 search incidents only #592

merged 12 commits into from Feb 15, 2018

Conversation

melinath
Copy link

@melinath melinath commented Feb 6, 2018

Made search page configurable sitewide as a link to an IncidentIndexPage. Set that up to be used across the board for search / recent incident lists instead of HomePage.incident_index_page.

Tradeoff: This means the search bar from the top of the page will no longer search for non-incident content. However, I think that the benefit of the more powerful incident search outweighs the loss of non-incident search, since this is a site primarily for press freedom incidents. I also think that this is what was being asked for, though I could be wrong.

Resolved #569.

@melinath melinath added this to Ready for Review in LW Issues Feb 6, 2018
@melinath melinath requested a review from raq929 February 6, 2018 16:41
@petersterne
Copy link

@melinath Just chiming in to say that you're correct. That's what I was asking for.

@melinath
Copy link
Author

melinath commented Feb 8, 2018

@petersterne great, thanks!

Copy link

@raq929 raq929 left a comment

Choose a reason for hiding this comment

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

Talk to me a little bit about your approach here?

@melinath
Copy link
Author

melinath commented Feb 9, 2018

I made the header search bar go to the "all incidents" page instead of a separate search view. This makes the user experience more fluid because it means that they have immediate access to the full power of the incident search interface, as well as the standard incident display, rather than getting a variety of content with very little styling or details.

In order to accomplish this, the all incidents page needed to be a site setting rather than a homepage attribute. And once this change was implemented, the search view became vestigial and removable.

Copy link

@raq929 raq929 left a comment

Choose a reason for hiding this comment

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

Looks goo d to me. I'm going to add an issue about the possibility of somehow adding blog posts to this search view eventually. Before you merge, can you put a note in the README about how search works, since it is now different from what I would expect as an incoming developer on the project?

@melinath melinath merged commit d249f48 into master Feb 15, 2018
LW Issues automation moved this from Ready for Review to Done Feb 15, 2018
@melinath melinath deleted the 569-search-incidents-only branch February 15, 2018 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
LW Issues
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants