From e4f74fa5289ec0e83cfde5ed605a24d275bf0df0 Mon Sep 17 00:00:00 2001
From: Ben Silverman <ben@performantsoftware.com>
Date: Wed, 24 Jul 2024 18:30:22 -0400
Subject: [PATCH] Redesign scholarship records page, include ITT panel (#1627)

---
 .../corpus/document_scholarship.html          | 36 +++++------
 .../corpus/snippets/document_tabs.html        |  2 +-
 geniza/corpus/templatetags/corpus_extras.py   |  8 ++-
 geniza/corpus/tests/test_corpus_templates.py  | 22 ++++---
 .../corpus/tests/test_corpus_templatetags.py  | 16 ++---
 sitemedia/js/controllers/iiif_controller.js   |  9 ++-
 sitemedia/scss/components/_docheader.scss     |  2 +-
 sitemedia/scss/components/_footnote.scss      | 63 +++++++++++++------
 sitemedia/scss/components/_tabs.scss          |  4 +-
 sitemedia/scss/pages/_document.scss           | 16 +++++
 sitemedia/scss/pages/_person.scss             |  2 +-
 sitemedia/scss/pages/_scholarship.scss        | 21 ++++++-
 12 files changed, 134 insertions(+), 67 deletions(-)

diff --git a/geniza/corpus/templates/corpus/document_scholarship.html b/geniza/corpus/templates/corpus/document_scholarship.html
index 397bf6022..812dd85ac 100644
--- a/geniza/corpus/templates/corpus/document_scholarship.html
+++ b/geniza/corpus/templates/corpus/document_scholarship.html
@@ -23,36 +23,36 @@ <h1 class="sr-only">{{ page_title }}</h1>
                             <dd>
                                 {{ source.grouper.formatted_display|safe }}
                             </dd>
-                            <dt class="relation">
-                                {# Translators: label for included document relations for a single footnote #}
-                                {% translate "includes" as includes_text %}
-                                {% if source.list|has_location_or_url %}
-                                    {# Translators: label for document relations in list of footnotes #}
-                                    {% blocktranslate with relation=source.list|all_doc_relations|lower trimmed %}
-                                        for {{ relation }} see
-                                    {% endblocktranslate%}
-                                {% else %}
-                                    {# Translators: label for document relations for one footnote with no location or URL #}
-                                    {% blocktranslate with relation=source.list|all_doc_relations|lower trimmed %}
-                                        includes {{ relation }}
-                                    {% endblocktranslate%}
-                                {% endif %}
-                            </dt>
                             {% if source.list|has_location_or_url %}
-                                <dd class="relation">
-                                    <ul>
+                                {# Translators: accessibility label for the specific location of a citation in a source record #}
+                                <dt class="sr-only">{% translate "Location in source" %}</dt>
+                                <dd>
+                                    <ul class="locations">
                                         {% for fn in source.list %}
                                             {% ifchanged %}{# omit duplicate footnote locations #}
-                                                <li class="location">{% include "corpus/snippets/footnote_location.html" %}</li>
+                                                <li>{% include "corpus/snippets/footnote_location.html" %}</li>
                                             {% endifchanged %}
                                         {% endfor %}
                                     </ul>
                                 </dd>
                             {% endif %}
+
+                            {# Translators: accessibility label for the relationship(s) of a source to a document #}
+                            <dt class="sr-only">{% translate "Relation to document" %}</dt>
+                            <dd>
+                                <ul class="relations">
+                                    {% for relation in source.list|all_doc_relations %}
+                                        <li>{{ relation }}</li>
+                                    {% endfor %}
+                                </ul>
+                            </dd>
                         </dl>
                     </li>
                 {% endspaceless %}
             {% endfor %}
         </ol>
     </div>
+
+    {# viewer #}
+    {% include "corpus/snippets/document_transcription.html" %}
 {% endblock main %}
diff --git a/geniza/corpus/templates/corpus/snippets/document_tabs.html b/geniza/corpus/templates/corpus/snippets/document_tabs.html
index afe1e69f0..8a52c39a5 100644
--- a/geniza/corpus/templates/corpus/snippets/document_tabs.html
+++ b/geniza/corpus/templates/corpus/snippets/document_tabs.html
@@ -8,7 +8,7 @@
             {% translate "Document Details" as details_text %}
             <li><a href="{{ document_url }}"{% if request.path == document_url %} aria-current="page"{% endif %}>{{ details_text }}</a></li>
             {# Translators: n_records is number of scholarship records #}
-            {% blocktranslate asvar srec_text %}Scholarship Records ({{ n_records }}){% endblocktranslate %}
+            {% blocktranslate asvar srec_text %}Select Bibliography ({{ n_records }}){% endblocktranslate %}
             {% url 'corpus:document-scholarship' pk=document.pk as scholarship_url %}
             <li>{% if n_records > 0 %}<a href="{{ scholarship_url }}"{% if request.path == scholarship_url %} aria-current="page"{% endif %}>{{ srec_text }}</a>{% else %}<span disabled aria-disabled="true">{{ srec_text }}</span>{% endif %}</li>
             {# Translators: n_reldocs is number of related documents #}
diff --git a/geniza/corpus/templatetags/corpus_extras.py b/geniza/corpus/templatetags/corpus_extras.py
index 6527b8cb4..0d84c155d 100644
--- a/geniza/corpus/templatetags/corpus_extras.py
+++ b/geniza/corpus/templatetags/corpus_extras.py
@@ -183,6 +183,8 @@ def has_location_or_url(footnotes):
 
 @register.filter
 def all_doc_relations(footnotes):
-    """For scholarship records list: join doc relations for all footnotes
-    by a comma."""
-    return ", ".join(sorted(set([str(fn.doc_relation) for fn in footnotes])))
+    """For scholarship records list: list doc relations for all footnotes."""
+    relations = set()
+    for fn in footnotes:
+        relations.update(set([n.strip() for n in str(fn.doc_relation).split(",")]))
+    return sorted(relations)
diff --git a/geniza/corpus/tests/test_corpus_templates.py b/geniza/corpus/tests/test_corpus_templates.py
index aa71d460d..698bfffdc 100644
--- a/geniza/corpus/tests/test_corpus_templates.py
+++ b/geniza/corpus/tests/test_corpus_templates.py
@@ -388,9 +388,7 @@ def test_source_relation(self, client, document, source, twoauthor_source):
         response = client.get(
             reverse("corpus:document-scholarship", args=[document.pk])
         )
-        assertContains(
-            response, '<dt class="relation">includes edition</dt>', html=True
-        )
+        assertContains(response, "<li>Edition</li>", html=True)
 
         fn2 = Footnote.objects.create(
             content_object=document,
@@ -401,7 +399,7 @@ def test_source_relation(self, client, document, source, twoauthor_source):
         response = client.get(
             reverse("corpus:document-scholarship", args=[document.pk])
         )
-        assertContains(response, '<dt class="relation">for edition see</dt>', html=True)
+        assertContains(response, "<li>Edition</li>", html=True)
 
         fn2.doc_relation = [Footnote.EDITION, Footnote.TRANSLATION]
         fn2.save()
@@ -410,7 +408,13 @@ def test_source_relation(self, client, document, source, twoauthor_source):
         )
         assertContains(
             response,
-            '<dt class="relation">for edition, translation see</dt>',
+            "<li>Edition</li>",
+            html=True,
+        )
+        print(response.content)
+        assertContains(
+            response,
+            "<li>Translation</li>",
             html=True,
         )
 
@@ -425,7 +429,7 @@ def test_source_location(self, client, document, source):
         response = client.get(
             reverse("corpus:document-scholarship", args=[document.pk])
         )
-        assertContains(response, "p. 25")
+        assertContains(response, "<li>p. 25</li>", html=True)
         fn.location = ""
         fn.save()
         response = client.get(
@@ -484,7 +488,7 @@ def test_no_footnotes(self, client, document):
         # uses span, not link
         assertContains(
             response,
-            "<span disabled aria-disabled='true'>Scholarship Records (0)</span>",
+            "<span disabled aria-disabled='true'>Select Bibliography (0)</span>",
             html=True,
         )
 
@@ -498,13 +502,13 @@ def test_with_footnotes(self, client, document, source, twoauthor_source):
             response, reverse("corpus:document-scholarship", args=[document.pk])
         )
         # count should be 1
-        assertContains(response, "Scholarship Records (1)")
+        assertContains(response, "Select Bibliography (1)")
 
         Footnote.objects.create(content_object=document, source=twoauthor_source)
         response = client.get(document.get_absolute_url())
 
         # count should be 2
-        assertContains(response, "Scholarship Records (2)")
+        assertContains(response, "Select Bibliography (2)")
 
     def test_no_related_docs(self, client, document, empty_solr):
         """document nav should render disabled related documents tab if no related documents"""
diff --git a/geniza/corpus/tests/test_corpus_templatetags.py b/geniza/corpus/tests/test_corpus_templatetags.py
index 47bf52aae..167e25e0e 100644
--- a/geniza/corpus/tests/test_corpus_templatetags.py
+++ b/geniza/corpus/tests/test_corpus_templatetags.py
@@ -59,10 +59,10 @@ def test_all_doc_relations(self, document, footnote):
             source=footnote.source,
             doc_relation=Footnote.DIGITAL_EDITION,
         )
-        assert (
-            corpus_extras.all_doc_relations(list(document.footnotes.all()))
-            == "Digital Edition, Edition"
-        )
+        assert corpus_extras.all_doc_relations(list(document.footnotes.all())) == [
+            "Digital Edition",
+            "Edition",
+        ]
         # should not repeat doc relations even if multiple of the same type appear
         Footnote.objects.create(
             object_id=document.pk,
@@ -71,10 +71,10 @@ def test_all_doc_relations(self, document, footnote):
             doc_relation=Footnote.EDITION,
             location="other place",
         )
-        assert (
-            corpus_extras.all_doc_relations(list(document.footnotes.all()))
-            == "Digital Edition, Edition"
-        )
+        assert corpus_extras.all_doc_relations(list(document.footnotes.all())) == [
+            "Digital Edition",
+            "Edition",
+        ]
 
 
 def test_dict_item():
diff --git a/sitemedia/js/controllers/iiif_controller.js b/sitemedia/js/controllers/iiif_controller.js
index 021c83ea6..f6ed62d6a 100644
--- a/sitemedia/js/controllers/iiif_controller.js
+++ b/sitemedia/js/controllers/iiif_controller.js
@@ -101,9 +101,12 @@ export default class extends Controller {
             );
         }
         const OSD = this.osdTarget.querySelector(".openseadragon-container");
-        OSD.style.transition = "opacity 300ms ease, visibility 0s ease 300ms";
-        OSD.style.visibility = "hidden";
-        OSD.style.opacity = "0";
+        if (OSD) {
+            OSD.style.transition =
+                "opacity 300ms ease, visibility 0s ease 300ms";
+            OSD.style.visibility = "hidden";
+            OSD.style.opacity = "0";
+        }
     }
 
     addOpenSeaDragon(settings) {
diff --git a/sitemedia/scss/components/_docheader.scss b/sitemedia/scss/components/_docheader.scss
index 61d4bc395..70af0de0b 100644
--- a/sitemedia/scss/components/_docheader.scss
+++ b/sitemedia/scss/components/_docheader.scss
@@ -37,7 +37,7 @@ html[lang="ar"] span#formatted-title {
 
 // Edit link for admins
 .edit-link-container {
-    @include container.measure;
+    max-width: calc(896px + 2rem);
     top: spacing.$spacing-4xl;
     position: absolute;
     width: 100%;
diff --git a/sitemedia/scss/components/_footnote.scss b/sitemedia/scss/components/_footnote.scss
index 2b396da65..0aaf34cc2 100644
--- a/sitemedia/scss/components/_footnote.scss
+++ b/sitemedia/scss/components/_footnote.scss
@@ -9,11 +9,10 @@
 
 .citation {
     counter-increment: search-counter;
-    padding-bottom: spacing.$spacing-md;
     padding: 0 0 spacing.$spacing-md spacing.$spacing-xl;
     margin-left: -#{spacing.$spacing-lg};
     @include breakpoints.for-tablet-landscape-up {
-        padding-left: spacing.$spacing-xl;
+        padding-left: 1.625rem;
         margin-left: 0;
     }
     @include typography.body;
@@ -26,28 +25,32 @@
     }
     &::before {
         content: counter(search-counter);
-        margin-left: -#{spacing.$spacing-xl};
+        margin-left: -1.625rem;
         text-align: left;
         float: left;
         @include typography.body-bold;
     }
-    .relation {
-        max-width: none; // required to span the whole width
-        background-color: var(--background-light);
-        margin-left: -#{spacing.$spacing-xl};
-        padding: spacing.$spacing-4xs 0 spacing.$spacing-4xs spacing.$spacing-xl;
-        &:is(dt) {
-            margin-top: spacing.$spacing-md;
-            @include typography.caption;
-        }
-        &:is(dd) {
-            margin-bottom: spacing.$spacing-2xs;
-            @include typography.meta;
-        }
-        li.location {
+    ul.locations {
+        margin-top: 0.5rem;
+        li {
+            @include typography.body-bold;
             word-break: break-all;
         }
     }
+    ul.relations {
+        display: flex;
+        flex-flow: row wrap;
+        gap: 1rem;
+        margin-top: 1rem;
+        li {
+            display: flex;
+            align-items: center;
+            height: 2rem;
+            padding: 0 spacing.$spacing-md;
+            border-radius: 5px;
+            background-color: var(--background-gray);
+        }
+    }
     div.unpublished {
         margin-top: spacing.$spacing-3xs;
         @include typography.unpublished;
@@ -55,13 +58,35 @@
     }
 }
 
+// RTL overrides
+html[dir="rtl"] .citation {
+    padding: 0 spacing.$spacing-xl spacing.$spacing-md 0;
+    margin-right: -#{spacing.$spacing-lg};
+    margin-left: 0;
+    @include breakpoints.for-tablet-landscape-up {
+        padding-right: 1.625rem;
+        padding-left: 0;
+        margin-right: 0;
+        margin-left: 0;
+    }
+    &::before {
+        margin-left: 0;
+        margin-right: -1.625rem;
+        text-align: right;
+        float: right;
+    }
+    & + .citation {
+        padding-top: spacing.$spacing-md;
+    }
+}
+
 // Hebrew variant
 html[lang="he"] .citation {
     @include typography.body-he;
     *[lang="en"] {
         @include typography.body;
     }
-    dd.relation:not([lang="en"]) {
+    ul.relations li:not([lang="en"]) {
         @include typography.meta-he;
     }
 }
@@ -72,7 +97,7 @@ html[lang="ar"] .citation {
     *[lang="en"] {
         @include typography.body;
     }
-    dd.relation:not([lang="en"]) {
+    ul.relations li:not([lang="en"]) {
         @include typography.meta-ar;
     }
 }
diff --git a/sitemedia/scss/components/_tabs.scss b/sitemedia/scss/components/_tabs.scss
index c18de9a1b..41f81bd14 100644
--- a/sitemedia/scss/components/_tabs.scss
+++ b/sitemedia/scss/components/_tabs.scss
@@ -63,8 +63,8 @@ nav#tabs {
         padding-bottom: spacing.$spacing-xs;
         border-bottom: 4px solid var(--tabs-bottom);
         @include breakpoints.for-tablet-landscape-up {
-            padding-left: 6rem;
-            padding-right: 6rem;
+            padding-left: 5.5rem;
+            padding-right: 5.5rem;
         }
     }
     // Link-specific tabs styling
diff --git a/sitemedia/scss/pages/_document.scss b/sitemedia/scss/pages/_document.scss
index 36ae8e3b7..59893be12 100644
--- a/sitemedia/scss/pages/_document.scss
+++ b/sitemedia/scss/pages/_document.scss
@@ -273,6 +273,22 @@ main.document {
     }
 }
 
+// RTL tweaks
+html[dir="rtl"] main.document section.content-stats ul {
+    margin-right: 1.25rem;
+    margin-left: 0;
+    li + li {
+        border-left: none;
+        border-right: 1px solid var(--disabled);
+        padding-left: 0;
+        padding-right: 1rem;
+        @include breakpoints.for-tablet-landscape-up {
+            padding-left: 0;
+            padding-right: 1.5rem;
+        }
+    }
+}
+
 // Hebrew variant
 html[lang="he"] main.document {
     .container section.description h3 {
diff --git a/sitemedia/scss/pages/_person.scss b/sitemedia/scss/pages/_person.scss
index eb652b676..53096a7d8 100644
--- a/sitemedia/scss/pages/_person.scss
+++ b/sitemedia/scss/pages/_person.scss
@@ -292,7 +292,7 @@ html[dir="rtl"] main.person,
 html[dir="rtl"] main.place,
 html[dir="rtl"] main.document {
     .container dl.metadata-list dt::before,
-    .container .description h2::before {
+    .container section > h2::before {
         margin-right: 0;
         margin-left: 12px;
     }
diff --git a/sitemedia/scss/pages/_scholarship.scss b/sitemedia/scss/pages/_scholarship.scss
index fa879f6b9..8f025ce8b 100644
--- a/sitemedia/scss/pages/_scholarship.scss
+++ b/sitemedia/scss/pages/_scholarship.scss
@@ -6,6 +6,13 @@
 @use "../base/spacing";
 
 main.scholarship {
+    max-width: 100vw;
+    @include breakpoints.for-tablet-landscape-up {
+        .container,
+        .container * {
+            max-width: calc(896px + 2rem);
+        }
+    }
     // Primary container
     div.container {
         display: flex;
@@ -17,9 +24,19 @@ main.scholarship {
     }
     ol {
         counter-reset: search-counter;
-        margin: spacing.$spacing-2xl 0 spacing.$spacing-xl spacing.$spacing-lg;
+        margin: 3rem 0 spacing.$spacing-xl spacing.$spacing-lg;
+        @include breakpoints.for-tablet-landscape-up {
+            margin: calc(2rem + 10px) 0 0;
+        }
+    }
+}
+
+// RTL overrides
+html[dir="rtl"] main.scholarship {
+    ol {
+        margin: 3rem spacing.$spacing-lg spacing.$spacing-xl 0;
         @include breakpoints.for-tablet-landscape-up {
-            margin: spacing.$spacing-3xl 0 0;
+            margin: calc(2rem + 10px) 0 0;
         }
     }
 }