Skip to content
This repository was archived by the owner on Sep 19, 2018. It is now read-only.

Even more search improvements - 2nd attempt :) #359

Merged
merged 25 commits into from
Oct 1, 2014

Conversation

sparkica
Copy link
Contributor

  • I moved search button to the top,
  • added bigger More button,
  • did some cleaning (removed unused code),
  • replaced search POST request with GET request: search url gets updated and can be bookmarked,
  • fixed pagination (no more custom endless js needed) and moved the text displaying number of results,
  • introduced history.js due to IE issues (https://github.com/browserstate/history.js),
  • replaced search parameter with q, so it is consistent with clickable searchable tags.

Sorry about creating a second PR, but the first one was a mess. @gandalfar please take a look at this one.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 7ff12f2 on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.55%) when pulling dceef98 on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.54%) when pulling dd1d4cc on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@ialja
Copy link
Contributor

ialja commented Sep 28, 2014

Looking good, great job! 👍

A few minor comments:

  • On the index page, we started using past=yes for the past filter, the new search uses past=on. Can you please switch back to past=yes for consistency?
  • As I'm using the real events website, I keep getting the feeling that we could display more search results per page. Maybe increase the number from 6 to 10?
  • Any special reason why you switched from "X events match your search criteria:" to "Events matching your search criteria: X"? I kinda feel the first wording is less robotic :)

@sparkica
Copy link
Contributor Author

  • Regarding 'past=yes' @gandalfar will have an answer, I don't :)
  • From 6-10: sure, no problem.
  • Yes, I agree it was less robotic. I changed it because number of results found has to be updated with every AJAX call (every click on any of the search criteria) via the script and it made more sense to just update the number itself rather than whole wording. However... this can be changed 😺

@ialja
Copy link
Contributor

ialja commented Sep 28, 2014

We've already shared links with past=yes, so it might be better to keep it that way :)

@jurecuhalev
Copy link
Contributor

I think it was something with serialization, but no problem @sparkica can easily change that constant.

@sparkica
Copy link
Contributor Author

@gandalfar Not quite sure what you're asking. Take a look at the History repo, this is from their documentation:

  • Provide a backwards-compatible experience for all HTML4 Browsers using a hash-fallback (including continued support for the HTML5 History API's data, title, pushState and replaceState) with the option to remove HTML4 support if it is not right for your application
  • Provide a forwards-compatible experience for HTML4 States to HTML5 States (so if a hash-fallbacked url is accessed by a HTML5 browser it is naturally transformed into its non-hashed url equivalent)

So if I understand correctly, we don't need any extra JS code to handle hashbangs.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.54%) when pulling 9317cb6 on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@jurecuhalev
Copy link
Contributor

@sparkica try this url in IE9: /search/#?q=&country=SI&past_events=on and you'll see that you don't get correct results because hashbang part of URL is not sent to server.

See detailed list of issues with hashbangs from their URL: https://github.com/browserstate/history.js/wiki/Intelligent-State-Handling

I would propose to drop html4 history rewriting support and either do full refresh of page url or not bother with history rewriting for old browsers.

@sparkica
Copy link
Contributor Author

@gandalfar Thanks for clarification (I don't have access to IE9 right now), I'll will figure something out.

@jurecuhalev
Copy link
Contributor

@sparkica download IE9 from https://www.modern.ie/en-us

@sparkica
Copy link
Contributor Author

Working on it 👍 Did a little research.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.54%) when pulling d8841cb on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@sparkica
Copy link
Contributor Author

@ialja I've replaced all the past='yes' values to past=on. When using GET and Django forms checkboxes, True is converted to 'on' and False to 'off' by default. This way we can avoid serialization issues.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.73%) when pulling a85457b on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.13%) when pulling d40d941 on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@sparkica
Copy link
Contributor Author

sparkica commented Oct 1, 2014

Well... it seems we already have modernizr included, I'll use it then.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) when pulling 62e9fe1 on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) when pulling 62e9fe1 on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.24%) when pulling 27782fd on sparkica:fea_search_get2 into cbcd4ae on codeeu:master.

ercchy added a commit that referenced this pull request Oct 1, 2014
Even more search improvements - 2nd attempt :)
@ercchy ercchy merged commit 8174e45 into codeeu:master Oct 1, 2014
@goranche
Copy link
Contributor

goranche commented Oct 1, 2014

this was reverted, because of bugs... please create a new PR (found issues will be written)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants