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

Added multi-tab search support for blacklight-4.3 #605

Closed
wants to merge 2 commits into from
Closed

Added multi-tab search support for blacklight-4.3 #605

wants to merge 2 commits into from

Conversation

elohanlon
Copy link
Contributor

Re-implemented multi-tab search logic using the latest Blacklight master. Resubmission of #552.

@cbeer
Copy link
Member

cbeer commented Sep 13, 2013

Thanks, I'll give this one a try.

@ghost ghost assigned cbeer Sep 13, 2013
@elohanlon
Copy link
Contributor Author

Actually, Travis is reporting a test failure (but I know why). I originally modified one of the now-gone cucumber tests, and needed to change the scope of a have_content call.

search_history_spec.rb:42 fails on:
expect(page).to_not have_content 'dang'

The problem is, it's scanning the entire page content for the word "dang", and because of the multi-tab fix, "dang" appears in a script in the < head > tag, which makes this test fail. We'd need to change the scope of this text scan so that it only includes the body tag. Is that okay?

@cbeer
Copy link
Member

cbeer commented Sep 16, 2013

This still isn't working for me.

In tab 1, I do a search for everything.

In tab 2, I click the language facet "Tibetan" (with 6 docs), and after I choose a document, the pagination info is:

« Previous | 3 of 30 | Next »

…history test more specific. Added test for multi-tab search.
@elohanlon
Copy link
Contributor Author

Ah, I see what you mean. Are you trying this in Rails 4? My original commit was for Blacklight on Rails 3, but I think that Turbolinks in Rails 4 is causing a problem. It's not refreshing my script tag in the head. I've relocated the script tag to the top of the body and I think that this should fix things. I've also re-added a non-cucumber version of my spec test from before, and I modified the search_history test that was throwing a false error after my changes.

Can you try again, using my latest commit?

(Also: Since my feature requires JavaScript, the test does too. Is the Blacklight Travis setup able to handle JS tests at this point?)

@cbeer
Copy link
Member

cbeer commented Sep 23, 2013

You called it, thanks, it seems to be working now, even with turbolinks enabled. I'd like to see if we can pare down the amount of information in the hash, and then I'm happy to get this merged.

I believe travis is configured to use phantomjs somehow. @jkeck?

@cbeer
Copy link
Member

cbeer commented Sep 23, 2013

Blocked by #619. I'll rebase this patch on top of #619 so we're not passing around so much information (especially controller internal information). That might even let us do a non-javascript alternative too..

@elohanlon
Copy link
Contributor Author

Sounds good!

@jcoyne
Copy link
Member

jcoyne commented Oct 18, 2013

Superseded by #619

@jcoyne jcoyne closed this Oct 18, 2013
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.

None yet

3 participants