search

NYULibraries / specialcollections

NYU Libraries Special Collections Discovery Application
https://specialcollections.library.nyu.edu
MIT License
8 stars 4 forks source link

Feature/stable repo links #48

Closed chrpr closed 9 years ago

chrpr commented 9 years ago

Hey @barnabyalter , @NYULibraries/finding-aids

I've deleted, re-based, and re-pushed my branch with a re-written version of this feature. This PR replaces #24 .

I think I addressed all of Barnaby's comments in that thread, and all my tests are passing.

I tried pulling out the coffeescript lines per Barnaby's suggestion, but the routes definitely don't affect the dropdown menu, just the actual search values.

I think I implemented to letter the feature request, but I have a question about the functionality for the product owners regarding a consequence of that implementation. I suspect that this will result in a new feature request. I'll put that info in Pivotal rather than here, though.

Let me know what you think, and whether you have any additional comments.

Thanks!

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.19%) to 96.94% when pulling 41c56932c846946064de8c7889c3228db29f07c4 on feature/stable_repo_links into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

chrpr commented 9 years ago

FYI -- I have no idea why that travis build failed. It's fine under the other 2 environments, and on my local setup. I don't believe I touched that feature at all.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.32%) to 97.46% when pulling 684cd33aec2b7100b7b37c9aa328778e2fa8ec88 on feature/stable_repo_links into 8bdc8be6668799c9dc2801f0553f3708cd10379b on development.

chrpr commented 9 years ago

Thanks @barnabyalter That's too funny, re: URL encoding. I didn't track down the true source of the problem because we only have data in ampersand-containing collection names. Good catch.

I've fixed this issue, rewritten the code a bit, and added the helper function specs you asked for. Can you have a look and see what you think?

Also, re the bug/feature, you're absolutely right. That's actually the issue that I was going to raise with the product owners. My initial idea had been to use the coffee to set the form action url to the correct route based on select, but I think I like where you're going with it better.

I'm now working on that coffeescript, and am a bit confused by your example. It looks like this says to set the dropdown selected value to whatever is in the repository header, but only if the current selection is "All Libraries"? I'm just not sure what that would do... I also don't really follow what you mean by, "As mentioned above you'll need to remove the search_field and you can use the Repositories class to DRY it up" Where do you mention removing the search_field above? Do you mean removing it from the coffeescript?

Sorry to be a bit dense, and thanks again for your feedback.

barnabyalter commented 9 years ago

@chrpr Remove the :search_field param from the route. Then only select the dropdown from the :repository param if no :search_field has been selected, i.e. 'All Libraries'. If you then select another dropdown option and submit the form the :search_field param will be set to that new value and so the coffeescript will not force the select to be the repos.

Unless of course desired functionality is that when you select an option from the dropdown and hit search that the default view becomes that repos, which makes sense but is not in the acceptance criteria.

chrpr commented 9 years ago

Closing this PR & replacing with a fresh one that reflects discussion at last sprint planning.