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

Simplify search filters #480

Merged
merged 6 commits into from
Jul 20, 2014
Merged

Simplify search filters #480

merged 6 commits into from
Jul 20, 2014

Conversation

monfresh
Copy link
Member

This replaces all the complicated view, JS, and controller code with 2
simple text fields for searching by location and agency name. This is a
familiar interface and keeps things simple and maintainable. Anything
that would be built on top of this simple foundation should be based on
user feedback.

Note that the text fields are not styled in this commit.

@monfresh
Copy link
Member Author

@anselmbradford Ready for review and styling, please! I think the search fields should be bigger than the default oval.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.68%) when pulling 6d18789 on simplify-filters into e538730 on master.

@anselmbradford
Copy link
Member

Here's the style updates direction:
screen shot 2014-07-17 at 11 59 00 pm

Also, might tackle #418 at the same time, and show clearing (x) all the time or on rollover, I'm not sure yet.

screen shot 2014-07-18 at 12 00 29 am

The existing placeholder text was too long, but I'll put what was there in the title attribute so it shows up on hover.

@anselmbradford
Copy link
Member

Also, the magnifying glass should be the same deep purple as the map-marker and building. Forgot to update that for the screenshot.

@monfresh
Copy link
Member Author

I think that'll work. A couple notes:

  1. Is it OK to not have a label for the input fields? Any accessibility considerations there?
  2. Please define all the search-related text in config/locales/en.yml so that it can be customized and translated. For the input field placeholders, I think it should look like this:
# in config/locales/en.yml
# note the single quotes around text to adhere to style guide
en:
  placeholders:
    keyword_search: 'Search for...'
    location_search: 'Near address or ZIP'

I think I already added a buttons key a few commits ago, so make sure to pull the latest branch code and then add entries for update_search_results and clear_search_fields. I'm not too crazy about "Remove search inputs". I think "Clear search fields" makes more sense, but then again, this is why we're making it easy to customize.

To access the values in the view, you use the t method, like this:

= t('placeholders.keyword_search')

@anselmbradford
Copy link
Member

The title attribute can used in place of the label. However, I've continued refining the design and brought the labels back:

screen shot 2014-07-18 at 11 22 04 am

How about just "clear search" for the reset button?

@anselmbradford
Copy link
Member

@declan let me know if you have any input on filter appearance direction.

@anselmbradford
Copy link
Member

And with values typed in:

screen shot 2014-07-18 at 11 57 27 am

@anselmbradford
Copy link
Member

Pushed some style updates.

Search sidebar:
screen shot 2014-07-19 at 1 01 27 pm

Geolocate icon change:
screen shot 2014-07-19 at 1 05 04 pm

Home icon change & minor padding/alignment adjustment of icons for details sidebar:
screen shot 2014-07-19 at 1 04 46 pm

I still need to do a sweep through different browsers to double-check the changes, but pushed the updates so they can be checked out.

@monfresh
Copy link
Member Author

Looks good, thanks! I noticed the magnifying glass in the sidebar is not deep purple as you mentioned. Did you forget to update it? The one on the home page turns purple on rollover. Also, I pushed some minor changes. Do a git pull first.

@anselmbradford
Copy link
Member

I didn't forget, I've been going back and forth on it. It clashes a bit with the logo. Do you prefer one or the other?

EDIT: typo

@monfresh
Copy link
Member Author

No preference, thanks for confirming. I'll fix conflicts with master and merge later today.

Adjusts component so that padding/margin can be added around inputs and
they won’t throw off the button positioning.
Adds hovers states for filter input buttons. Adjusts homepage search
button icon color to match inside hover state.
Adds padding and margin settings to input filter buttons, which were
being thrown off otherwise on iOS. Adds default border radius of 0 in
webkit-reset mixin as this is needed to remove default rounded corners
in iOS5+.
Updates font awesome icon on no search results view to match that used
in the search sidebar.
@monfresh
Copy link
Member Author

I merged this branch into my local master branch, resolved the conflicts, then pushed to GitHub. I thought GitHub would pick up on that but apparently that's not the GitHub way to do it. My bad. I'll close this PR now that it has actually been merged.

@monfresh monfresh closed this Jul 20, 2014
@monfresh
Copy link
Member Author

Argh, I just noticed @anselmbradford pushed more commits recently. I didn't pick those up. Next time, please add a comment. I wish GitHub would include PR commits in their email notifications.

@monfresh monfresh reopened this Jul 20, 2014
@anselmbradford
Copy link
Member

@monfresh sorry about that. I was doing a test sweep through different browsers and got pulled away. Will post a comment next time.

@monfresh monfresh merged commit 83bfe43 into master Jul 20, 2014
@monfresh
Copy link
Member Author

We're all good now.

@monfresh monfresh deleted the simplify-filters branch July 20, 2014 21:47
@anselmbradford
Copy link
Member

Any idea why clicking "Medicare" on http://ohana-web-search-demo.herokuapp.com causes an unstyled page?

@monfresh
Copy link
Member Author

It has nothing to do with what keyword you clicked on. If you look at the console, you'll see application.css can't be found. Will push to Heroku again and see if it fixes it.

@monfresh
Copy link
Member Author

For some reason, certain pages (like that Medicare one) are trying to find a different application.css. The digest is different on that page than on pages that work.

@monfresh
Copy link
Member Author

Maybe the Memcached cache needs to be flushed.

@monfresh
Copy link
Member Author

I think that did the trick. Clear your browser's cache and try again.

@declan
Copy link
Contributor

declan commented Jul 21, 2014

@anselmbradford I think this looks great.

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.

4 participants