Update record view for GDT #135
Conversation
Pull Request Test Coverage Report for Build 8302069999Details
💛 - Coveralls |
app/helpers/record_helper.rb
Outdated
| def coverage_date(dates) | ||
| return if dates.blank? || dates&.none? { |date| date['kind'] == 'Coverage' } | ||
|
|
||
| # If there is more than one coverage date, take the first one. |
There was a problem hiding this comment.
I'm not sure this is the behavior we want. I feel we should display multiple dates in this situation, either as comma separate or as a bulleted list.
There was a problem hiding this comment.
What if I told you this is common?
[{"kind"=>"Coverage", "note"=>nil, "range"=>nil, "value"=>"2020"}, {"kind"=>"Coverage", "note"=>nil, "range"=>{"gte"=>"2020", "lte"=>"2020"}, "value"=>nil}]
(Half)-kidding aside, I think it's a good idea to include all of them.
There was a problem hiding this comment.
Neat. I think this is a good example we may need to bring to DataEng and/or UX to decide if we want to change the data mappings/better understand the data mappings/better understand what we want to do on the UI side.
For now, if you want to build in duplicate detection logic in the UI I could get behind it. I'm reluctant to just assume all dates are duplicate (without more insight from DataEng as to how these mappings are created at least).
There was a problem hiding this comment.
Yeah, I generally don't like putting that kind of logic in UI, but it seems like a reasonable solution in this case.
There was a problem hiding this comment.
I feel like it's still worth a convo between EngX/DataEng at some point to help us better understand all the assumptions about dates. But for now this looks good to me.
app/helpers/record_helper.rb
Outdated
| def issued_date(dates) | ||
| return if dates.blank? || dates&.none? { |date| date['kind'] == 'Issued' } | ||
|
|
||
| # If there is more than one date issued, take the first one. |
There was a problem hiding this comment.
I'm not sure this is the behavior we want. I feel we should display multiple dates in this situation, either as comma separate or as a bulleted list.
| @@ -0,0 +1,3 @@ | |||
| <% if url_for(:back)&.starts_with?([root_url, 'results'].join('')) %> | |||
| @@ -0,0 +1,14 @@ | |||
| <ul class="list-inline"> | |||
| <li><%= metadata['contentType']&.each { |type| type['value'] }&.join(' ; ') %></li> | |||
There was a problem hiding this comment.
I noted this in slack, but I think the css rules need to be updated for these <li> tags to include something like:
vertical-align: top;
to address weird alignment issues.
There was a problem hiding this comment.
Unless we've heard otherwise that we want to silently drop multiple dates, I think we should update the logic to display the multiple values.
I have a suggestion on how to handle the weird alignment issues we can see in Safari we should either include in this or open as a ticket in the backlog so it doesn't keep popping up.
Why these changes are being introduced: The GeoData app has specific design requirements for the record view. Relevant ticket(s): * [GDT-131](https://mitlibraries.atlassian.net/browse/GDT-131) * [GDT-130](https://mitlibraries.atlassian.net/browse/GDT-130) How this addresses that need: This adds a `record_geo` partial that includes metadata specific to GDT records, such as locations and a metadata download link. It also changes where the `access_button` partial is displayed depending on screen width, and it adds a 'back' button to the top of the record if `url_for(:back)` resembles a search results page. Side effects of this change: * Some of the changes introduced in GDT-130 have been overwritten by this commit, as certain styles and markup are shared by both views. Specifically, the `geo_data_info` and `_authors` partials have been moved to the shared directory, and a new `shared` SCSS partial contains styles used in `result` and `record`. * Several helper methods have been added to parse the metadata needed for the record view.
Why these changes are being introduced:
The GeoData app has specific design requirements for the record view.
Relevant ticket(s):
How this addresses that need:
This adds a
record_geopartial that includes metadata specific to GDT records, such as locations and a metadata download link. It also changes where theaccess_buttonpartial is displayed depending on screen width, and it adds a 'back' button to the top of the record ifurl_for(:back)resembles a search results page.Side effects of this change:
geo_data_infoand_authorspartials have been moved to the shared directory, and a newsharedSCSS partial contains styles used inresultand `record.Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO