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

Homepage #21

Conversation

anusharanganathan
Copy link
Collaborator

This is the same as PR #20, with the commits squashed. Hope this is easier to compare.

Homepage designed as per page 2 of design specification

@jkeck
Copy link
Contributor

jkeck commented Mar 17, 2016

@anusharanganathan so I may be missing an important step here. When I look at this branch (at sha f82fc6f) I see something very different from what is on the instance that @blalbrit shared with me. I have some fixture data loaded, so I understand that I won't get the full experience as in that instance with real data, but the apps appear to be styled completely differently (way different search bar, no fixed nav-bar, etc).

Local

colligo-local

Server

colligo-server

Should I be expecting to see what I'm getting locally or what is in the deployed instance?

@anusharanganathan
Copy link
Collaborator Author

@jkeck you should be expecting what you are seeing locally. I did most of the style refinements at the end. The search bar on the home page should be similar to the deployed instance (4 tabs with search box and button), except for the name of the first tab and the styling of the button.

@blalbrit
Copy link
Collaborator

Hi @anusharanganathan - just for clarification, are those style refinements something you'll be pushing up in a follow-on pull request? We really like them and are hoping to see them in the final codebase.

@jkeck
Copy link
Contributor

jkeck commented Mar 17, 2016

Okay @anusharanganathan, totally understand.

One thing I notice is that when I limit using the slider I get an error

slider-error

It looks to me like the facet isn't configured? Not exactly sure, I didn't dig deeply into it.

Also, the fixture indexer is broken (although I do note that wasn't part of this PR, but it is when I noticed it). I get an error when trying to index fixtures: NameError: uninitialized constant DataIndexer::SolrDocument

I was able to solve this locally by making sure the rake task has a dependency on the rails environment. Would you like me to submit a ticket for that (alternatively, I'm more than happy to submit a PR directly to this repo to address that issue, but I don't know if you want to have that change come upstream from your repo).

@anusharanganathan
Copy link
Collaborator Author

@blalbrit yes, I will be pushing all of the style changes in a later PR. When I was coding I first worked on the functionality of each of the pages, following the order of the design document and once all of the functionality was in place, I concentrated on UI updates. The PRs will likely follow that order.

@anusharanganathan
Copy link
Collaborator Author

@jkeck I am able to reproduce the error you have with the fixture indexer and I have fixed it. Thanks. I also noticed two other things

  1. Do I also need rake jetty:environment in the download_and_unzip_jetty task after rake jetty:unzip
  2. When I run the fixtures task, I assume jetty is running. If Jetty isn't running, it will throw a connection error. Do I also start jetty as a part of fixtures - for example do something like
begin
  DataIndexer.new('Test collection', 'data/test_manifest_urls.csv').run
rescue Errno::ECONNREFUSED
  Rake::Task['jetty:start'].invoke
  DataIndexer.new('Test collection', 'data/test_manifest_urls.csv').run
end

I don't want to add the starting of jetty as part of the install task, as I want the solr config files copied before jetty is started.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 98.264% when pulling 4159064 on anusharanganathan:feature_homepage_2 into d441e82 on sul-dlss:master.

@anusharanganathan
Copy link
Collaborator Author

@jkeck - I have also fixed the facet error. Thanks a lot for spotting it.

@jkeck
Copy link
Contributor

jkeck commented Jun 27, 2016

Superseded by #22

@jkeck jkeck closed this Jun 27, 2016
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

4 participants