Isolate search forms for visual design changes #249
Conversation
jazairi
force-pushed
the
use-127-refactor-search-form
branch
from
85237f3 to
b9dee49
Compare
jazairi
changed the title
Pull Request Test Coverage Report for Build 18750569038Details
💛 - Coveralls |
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
This all looks fine, my only non-blocking question is about whether we need some additional assertions in the test suite to confirm we don't leak some extra templates as logic changes down the line.
If there's an aspect of the longer term strategy that makes these assertions unnecessary, or this doesn't seem necessary, no worries.
| assert_response :success | ||
|
|
||
| # Should show basic form without geo elements | ||
| assert_select 'form#search-form', { count: 1 } |
There was a problem hiding this comment.
Is it worth having an assertion to confirm #search-form-geo isn't present, to keep the reverse pattern from below?
assert_select 'form#search-form-geo', { count: 0 }
| mock_timdex_search_success | ||
| get '/results?q=test' | ||
| assert_response :success | ||
| assert_select 'form#search-form-geo', { count: 1 } |
There was a problem hiding this comment.
Same basic question as above - should there be an assertion that form#search-form doesn't exist in this test?
There was a problem hiding this comment.
Agreed -- and it just occurred to me that the GeoData tests should go in the search_controller_geo_test file instead.
|
@matt-bernhardt Thanks for the review! I think the latest commit addresses your feedback -- could you confirm? I also moved the GeoData-specific tests to the search_controller_geo_test file. (Forgot that we use separate files for GeoData and non-GeoData controller tests.) |
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
Thanks! - moving the geo tests is a good call too.
Why these changes are being introduced: GeoData and USE UI currently share search form input and submit fields. These fields should be isolated for UXWS to easily make tweaks to the form for USE UI that won't affect GeoData. Relevant ticket(s): * [USE-127](https://mitlibraries.atlassian.net/browse/USE-127) How this addresses that need: This creates separate form partials for USE and GeoData to faciliate changes to the USE form without affecting GeoData. It also moves the USE search form to the header, which is where UXWS' designs have it. Side effects of this change: It's possible we may want to move the GeoData form to the header as well, but given the tight timeline for USE UI beta, it makes sense to focus on the USE side of things for now.
jazairi
force-pushed
the
use-127-refactor-search-form
branch
from
e46054a to
6b3056b
Compare
Why these changes are being introduced:
GeoData and USE UI currently share search form input and submit fields.
These fields should be isolated for UXWS to easily make tweaks to the
form for USE UI that won't affect GeoData.
Relevant ticket(s):
How this addresses that need:
This creates separate form partials for USE and GeoData to faciliate
changes to the USE form without affecting GeoData. It also moves the
USE search form to the header, which is where UXWS' designs have it.
Side effects of this change:
It's possible we may want to move the GeoData form to the header as
well, but given the tight timeline for USE UI beta, it makes sense to
focus on the USE side of things for now.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
Please check both GeoData and USE. There should be no changes to the GeoData form. The USE form won't look right, but that's okay as Dave will be working on it shortly.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing