Skip to content

Commit

Permalink
Address code review feedback
Browse files Browse the repository at this point in the history
Matt noted two issues with the changeset:

1. It unintentionally removes the `truncated-list` class from the
contributors list in the results.
2. The `list-inline` class provided by the style library styles
list items with `inline-block`, not `inline`.

When I went to fix issue 1, I realized that issue 2 impacts it:
i.e., if the list items are `inline-block`, then `line-clamp` will
not truncate them properly.

This commit reintroduces `truncated-list` to the contributors list
in the results views and adds a rule to display items inline.
(The `list-inline` class is still applied, as it also removes the
left margin from the `ul`.)

This also changes how contributor kinds are rendered by refactoring
that logic to a helper method and handling some edge cases. This
is unrelated to the code review discussion, but I noticed it while
fixing the CSS issues.
  • Loading branch information
jazairi committed May 20, 2024
1 parent b224418 commit 903bac8
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 13 deletions.
1 change: 1 addition & 0 deletions app/assets/stylesheets/partials/_results.scss
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@
display: -webkit-box;
-webkit-line-clamp: 3;
-webkit-box-orient: vertical;
text-overflow: ellipsis;
}

.inner-heading {
Expand Down
3 changes: 3 additions & 0 deletions app/assets/stylesheets/partials/_shared.scss
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
font-size: 1.6rem;
font-weight: $fw-bold;
margin-bottom: 0.6em;
li {
display: inline;
}
li::after {
content: " ; ";
}
Expand Down
9 changes: 9 additions & 0 deletions app/helpers/application_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,15 @@ def record_page_title(record, character_limit = 25)
"#{title} | #{page_title_base}"
end

# Since this is used by both the result and record view, it is included in the application helper rather than one of
# those helpers.
def render_contributor_type(type)
return if type == 'Not specified'

type = 'Author' if type == 'mitauthor'
"#{type.humanize}: "
end

private

def page_title_base
Expand Down
5 changes: 4 additions & 1 deletion app/views/record/_record_geo.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@
</div>

<% if @record['contributors'].present? %>
<%= render partial: 'shared/contributors', locals: { contributors: @record['contributors'] } %>
<span class="sr">Contributors: </span>
<ul class="list-inline contributors">
<%= render partial: 'shared/contributors', locals: { contributors: @record['contributors'] } %>
</ul>
<% end %>
<% if @record['alternateTitles'].present? %>
Expand Down
5 changes: 4 additions & 1 deletion app/views/search/_result.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
</span>
</p>

<%= render partial: 'shared/contributors', locals: { contributors: result['contributors'] } %>
<span class="sr">Contributors: </span>
<ul class="list-inline truncate-list contributors">
<%= render partial: 'shared/contributors', locals: { contributors: result['contributors'] } %>
</ul>

<div class="result-highlights">
<%= render partial: 'search/highlights', locals: { result: result } %>
Expand Down
5 changes: 4 additions & 1 deletion app/views/search/_result_geo.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
</div>

<% if result_geo['contributors'] %>
<%= render partial: 'shared/contributors', locals: { contributors: result_geo['contributors'] } %>
<span class="sr">Contributors: </span>
<ul class="list-inline truncate-list contributors">
<%= render partial: 'shared/contributors', locals: { contributors: result_geo['contributors'] } %>
</ul>
<% end %>
<% if result_geo['summary'] %>
Expand Down
12 changes: 2 additions & 10 deletions app/views/shared/_contributors.html.erb
Original file line number Diff line number Diff line change
@@ -1,16 +1,8 @@
<%= return if contributors.blank? %>
<% contribs_list = [] %>
<% contributors&.each do |contributor| %>
<% contribs_list << contributor %>
<% end %>

<span class="sr">Contributors: </span>
<ul class="list-inline contributors">
<% contribs_list.uniq.map do |contributor| %>
<% contributors.uniq.map do |contributor| %>
<li>
<%= "#{contributor['kind'].titleize}: " %><%= link_to contributor['value'],
<%= render_contributor_type(contributor['kind']) %><%= link_to contributor['value'],
results_path({ advanced: true, contributors: contributor['value'] }) %>
</li>
<% end %>
</ul>
15 changes: 15 additions & 0 deletions test/helpers/application_helper_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,19 @@ class ApplicationHelperTest < ActionView::TestCase
assert_equal 'bar | GeoData | MIT Libraries', record_page_title(record)
end
end

test 'render_contributor_type ignores unspecified kinds' do
unspecified_type = 'Not specified'
assert_nil render_contributor_type(unspecified_type)
end

test 'render_contributor_type translates mitauthor' do
mitauthor_type = 'mitauthor'
assert_equal 'Author: ', render_contributor_type(mitauthor_type)
end

test 'render_contributor_type humanizes inputs' do
type = 'copy editor'
assert_equal 'Copy editor: ', render_contributor_type(type)
end
end

0 comments on commit 903bac8

Please sign in to comment.