Merge pull request #396 from trln/TD-1264-bugfix-dd-metadata

Bugfix for wayward dd elements appearing throughout metadata displays…

VERSION

@@ -1 +1 @@
-2.2.3
+2.2.4

app/components/trln_argon/document_header_metadata_component.html.erb

@@ -1,5 +1,8 @@
-<ul>
+<dl>
   <% @fields.each do |field| -%>
-    <%= @view_context.render(field_component(field).new(field: field, show: @show, layout: TrlnArgon::MetadataHeaderFieldLayoutComponent)) %>
+    <%= @view_context.render(field_component(field).new(
+          field: field,
+          show: @show,
+          layout: TrlnArgon::MetadataHeaderFieldLayoutComponent)) %>
   <% end -%>
-</ul>
+</dl>

app/components/trln_argon/document_metadata_list_component.html.erb

@@ -1,5 +1,5 @@
-<ul>
+<dl>
   <% @fields.each do |field| -%>
     <%= @view_context.render(field_component(field).new(field: field, show: @show, layout: TrlnArgon::MetadataListFieldLayoutComponent)) %>
   <% end -%>
-</ul>
+</dl>

app/components/trln_argon/metadata_field_layout_component.html.erb

@@ -1,4 +1,6 @@
 <div>
   <dt class="blacklight-<%= @key %> <%= @label_class %>"><%= label %></dt>
-  <dd class="blacklight-<%= @key %> <%= @value_class %>"><%= value %></dd>
-</div>
+  <% values.each do |v| %>
+    <%= v %>
+  <% end %>
+</div>

app/components/trln_argon/metadata_field_layout_component.rb

@@ -1,16 +1,11 @@
 module TrlnArgon
   class MetadataFieldLayoutComponent < Blacklight::MetadataFieldLayoutComponent
-
     # @param field [Blacklight::FieldPresenter]
     def initialize(field:, label_class: '', value_class: '')
       @field = field
       @key = @field.key.parameterize
       @label_class = label_class
       @value_class = value_class
     end
-
-    def render?
-      value.present?
-    end
   end
 end

app/components/trln_argon/metadata_header_field_layout_component.html.erb

@@ -1,3 +1,4 @@
-<li class="index-metadata <%= @key  %>">
-  <%= value %>
-</li>
+<dt class="blacklight-<%= @key %> <%= @label_class %>"><%= label %></dt>
+<% values.each do |v| %>
+  <%= v %>
+<% end %>

app/components/trln_argon/metadata_header_field_layout_component.rb

@@ -1,12 +1,20 @@
 module TrlnArgon
   class MetadataHeaderFieldLayoutComponent < Blacklight::MetadataFieldLayoutComponent
-    # rubocop:disable Style/RedundantParentheses
+    def initialize(field:, label_class: 'sr-only', value_class: '')
+      super
+      @field = field
+      @key = @field.key.parameterize
+      @label_class = label_class
+      @value_class = value_class
+    end
+
+    # values is an Array of ViewComponent::SlotV2
+    # objects, each containing html markup, some with only
+    # \n for content. So this requires gymnastics.
     def render?
-      # value is actually a ViewComponent::SlotV2 and is probably
-      # not nil?
-      # Rubocop lies, I find this clearer
-      !(value&.to_s.blank?)
+      values.map(&:to_s)
+            .map { |v| strip_tags(v).squish }
+            .compact_blank.present?
     end
-    # rubocop:enable Style/RedundantParentheses
   end
 end

app/components/trln_argon/metadata_list_field_layout_component.html.erb

@@ -1 +1,4 @@
-<%= value %>
+<dt class="blacklight-<%= @key %> <%= @label_class %>"><%= label %></dt>
+<% values.each do |v| %>
+  <%= v %>
+<% end %>

app/components/trln_argon/metadata_list_field_layout_component.rb

@@ -1,7 +1,11 @@
 module TrlnArgon
   class MetadataListFieldLayoutComponent < Blacklight::MetadataFieldLayoutComponent
-    def render?
-      value.present?
+    def initialize(field:, label_class: 'sr-only', value_class: '')
+      super
+      @field = field
+      @key = @field.key.parameterize
+      @label_class = label_class
+      @value_class = value_class
     end
   end
 end

app/views/catalog/_show_sub_header_default.html.erb

@@ -1,3 +1,3 @@
 <div id="show-sub-header" class="<%= show_sub_header_class %>">
   <%= render(TrlnArgon::DocumentHeaderMetadataComponent.new(fields: show_sub_header_presenter(document).field_presenters)) %>
-</div>
+</div>

lib/trln_argon/view_helpers/names_helper.rb

@@ -2,9 +2,11 @@ module TrlnArgon
   module ViewHelpers
     module NamesHelper
       def names_display(options = {})
-        assemble_names_list_value(options[:value]).map do |work|
-          content_tag(:li, work.html_safe, class: options[:field])
-        end .join('').html_safe
+        content_tag(:ul) do
+          assemble_names_list_value(options[:value]).map do |work|
+            content_tag(:li, work.html_safe, class: options[:field])
+          end .join('').html_safe
+        end
       end
 
       def assemble_names_list_value(value)

lib/trln_argon/view_helpers/subjects_helper.rb

@@ -2,11 +2,13 @@ module TrlnArgon
   module ViewHelpers
     module SubjectsHelper
       def list_of_linked_subjects_segments(options = {})
-        link_to_subject_segments(options).map do |subject|
-          content_tag :li, class: options[:field] do
-            progress_link_span(subject)
-          end
-        end.join('').html_safe
+        content_tag :ul do
+          link_to_subject_segments(options).map do |subject|
+            content_tag :li, class: options[:field] do
+              progress_link_span(subject)
+            end
+          end.join('').html_safe
+        end
       end
 
       def list_of_linked_genres_segments(options = {})

lib/trln_argon/view_helpers/work_entry_helper.rb

@@ -2,11 +2,13 @@ module TrlnArgon
   module ViewHelpers
     module WorkEntryHelper
       def included_works_display(options = {})
-        assemble_work_entry_list_value(options[:value]).map do |work|
-          content_tag :li, class: options[:field] do
-            content_tag :span, work.html_safe, class: 'progressive-link-wrapper'
-          end
-        end.join('').html_safe
+        content_tag :ul do
+          assemble_work_entry_list_value(options[:value]).map do |work|
+            content_tag :li, class: options[:field] do
+              content_tag :span, work.html_safe, class: 'progressive-link-wrapper'
+            end
+          end.join('').html_safe
+        end
       end
 
       # TODO: a bit of a departure from

spec/features/brief_records_spec.rb

@@ -13,15 +13,19 @@ def brief_records
     end
 
     it 'displays the statement of responsibility field value' do
-      expect(brief_records).to have_css('li.statement_of_responsibility_a', text: /.+/)
+      expect(brief_records).to have_css('dd.blacklight-statement_of_responsibility_a', text: /.+/)
+    end
+
+    it 'does not display a blank edition field value' do
+      expect(brief_records).not_to have_css('.document[data-document-id=UNCb3917352] dd.blacklight-edition_a')
     end
 
     it 'displays the imprint main field value' do
-      expect(brief_records).to have_css('li.imprint_main_a', text: /.+/)
+      expect(brief_records).to have_css('dd.blacklight-imprint_main_a', text: /.+/)
     end
 
     it 'displays the resource_type field value' do
-      expect(brief_records).to have_css('li.resource_type_a', text: /.+/)
+      expect(brief_records).to have_css('dd.blacklight-resource_type_a', text: /.+/)
     end
   end
 

spec/features/full_records_spec.rb

@@ -42,7 +42,7 @@
     end
 
     it 'displays the edition statement' do
-      expect(page).to have_css('li.edition_a', text: /5th/)
+      expect(page).to have_css('dd.blacklight-edition_a', text: /5th/)
     end
   end
 end

spec/lib/trln_argon/view_helpers/trln_argon_helper_spec.rb

@@ -1070,8 +1070,8 @@
 
     it 'creates a display value with links from the supplied names data' do
       expect(context.helpers.names_display(options)).to eq(
-        '<li><a href="/catalog?search=something">Nabokov, Vladimir Vladimirovich, 1899-1977</a>, author</li>'\
-        '<li><a href="/catalog?search=something">Appel, Alfred</a></li>'
+        '<ul><li><a href="/catalog?search=something">Nabokov, Vladimir Vladimirovich, 1899-1977</a>, author</li>'\
+        '<li><a href="/catalog?search=something">Appel, Alfred</a></li></ul>'
       )
     end
   end
@@ -1296,7 +1296,7 @@
 
     it 'creates a display value with links from the supplied work entry data that are formatted as an unordered list' do
       expect(context.helpers.included_works_display(options)).to eq(
-        '<li>'\
+        '<ul><li>'\
         '<span class="progressive-link-wrapper">'\
         '<a class="progressive-link" href="/catalog?search=something">Author</a>'\
         '<a class="progressive-link" href="/catalog?search=something">'\
@@ -1315,7 +1315,7 @@
         '<span class="sr-only">Author One Two Three Four</span> Four'\
         '</a>'\
         '</span>'\
-        '</li>'
+        '</li></ul>'
       )
     end
   end