From 0f2626868bd635f44f2c04d57a5e8ced1229737e Mon Sep 17 00:00:00 2001
From: Simon Willison <swillison@gmail.com>
Date: Wed, 30 Sep 2020 16:01:19 -0700
Subject: [PATCH] Much improved column menu display logic, refs #981

* Menu links now take into account existing querystring
* No longer shows facet option for primary key columns
* Conditionally displays sort/sort-desc if already sorted
* Does not show facet option if already faceted by this
---
 datasette/static/table.js       | 52 +++++++++++++++++++++++++++------
 datasette/templates/_table.html |  2 +-
 datasette/views/table.py        | 14 +++++++--
 tests/test_html.py              | 32 +++++++++++++++++---
 4 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/datasette/static/table.js b/datasette/static/table.js
index 0f50dcf5..a5f21979 100644
--- a/datasette/static/table.js
+++ b/datasette/static/table.js
@@ -13,16 +13,37 @@ var DROPDOWN_ICON_SVG = `<svg xmlns="http://www.w3.org/2000/svg" width="14" heig
 </svg>`;
 
 (function() {
+    // Feature detection
+    if (!window.URLSearchParams) {
+        return;
+    }
+    function getParams() {
+        return new URLSearchParams(location.search);
+    }
+    function paramsToUrl(params) {
+        var s = params.toString();
+        return s ? ('?' + s) : '';
+    }
     function sortDescUrl(column) {
-        return '?_sort_desc=' + encodeURIComponent(column);
+        var params = getParams();
+        params.set('_sort_desc', column);
+        params.delete('_sort');
+        return paramsToUrl(params);
     }
     function sortAscUrl(column) {
-        return '?_sort=' + encodeURIComponent(column);
+        var params = getParams();
+        params.set('_sort', column);
+        params.delete('_sort_desc');
+        return paramsToUrl(params);
     }
     function facetUrl(column) {
-        return '?_facet=' + encodeURIComponent(column);
+        var params = getParams();
+        params.append('_facet', column);
+        return paramsToUrl(params);
+    }
+    function isFacetedBy(column) {
+        return getParams().getAll('_facet').includes(column);
     }
-
     function iconClicked(ev) {
         ev.preventDefault();
         var th = ev.target;
@@ -33,12 +54,25 @@ var DROPDOWN_ICON_SVG = `<svg xmlns="http://www.w3.org/2000/svg" width="14" heig
         var menuTop = rect.bottom + window.scrollY;
         var menuLeft = rect.left + window.scrollX;
         var column = th.getAttribute('data-column');
-        menu.querySelector('a.dropdown-sort-desc').setAttribute('href', sortDescUrl(column));
-        menu.querySelector('a.dropdown-sort-asc').setAttribute('href', sortAscUrl(column));
-        /* Only show facet if it's not the first column */
-        var isFirstColumn = th.parentElement.querySelector('th:first-of-type') == th;
+        var params = getParams();
+        var sort = menu.querySelector('a.dropdown-sort-asc');
+        var sortDesc = menu.querySelector('a.dropdown-sort-desc');
         var facetItem = menu.querySelector('a.dropdown-facet');
-        if (isFirstColumn) {
+        if (params.get('_sort') == column) {
+            sort.style.display = 'none';
+        } else {
+            sort.style.display = 'block';
+            sort.setAttribute('href', sortAscUrl(column));
+        }
+        if (params.get('_sort_desc') == column) {
+            sortDesc.style.display = 'none';
+        } else {
+            sortDesc.style.display = 'block';
+            sortDesc.setAttribute('href', sortDescUrl(column));
+        }
+        /* Only show facet if it's not the first column, not selected, not a PK */
+        var isFirstColumn = th.parentElement.querySelector('th:first-of-type') == th;
+        if (isFirstColumn || params.getAll('_facet').includes(column) || th.getAttribute('data-is-pk') == '1') {
             facetItem.style.display = 'none';
         } else {
             facetItem.style.display = 'block';
diff --git a/datasette/templates/_table.html b/datasette/templates/_table.html
index 1cb4110b..8b933482 100644
--- a/datasette/templates/_table.html
+++ b/datasette/templates/_table.html
@@ -3,7 +3,7 @@
         <thead>
             <tr>
                 {% for column in display_columns %}
-                    <th class="col-{{ column.name|to_css_class }}" scope="col" data-column="{{ column.name }}">
+                    <th class="col-{{ column.name|to_css_class }}" scope="col" data-column="{{ column.name }}" data-is-pk="{% if column.is_pk %}1{% else %}0{% endif %}">
                         {% if not column.sortable %}
                             {{ column.name }}
                         {% else %}
diff --git a/datasette/views/table.py b/datasette/views/table.py
index 74126244..245bac9c 100644
--- a/datasette/views/table.py
+++ b/datasette/views/table.py
@@ -91,10 +91,18 @@ class RowTableShared(DataView):
         db = self.ds.databases[database]
         table_metadata = self.ds.table_metadata(database, table)
         sortable_columns = await self.sortable_columns_for_table(database, table, True)
-        columns = [
-            {"name": r[0], "sortable": r[0] in sortable_columns} for r in description
-        ]
         pks = await db.primary_keys(table)
+        pks_for_display = pks
+        if not pks_for_display:
+            pks_for_display = ["rowid"]
+        columns = [
+            {
+                "name": r[0],
+                "sortable": r[0] in sortable_columns,
+                "is_pk": r[0] in pks_for_display,
+            }
+            for r in description
+        ]
         column_to_foreign_key_table = {
             fk["column"]: fk["other_table"]
             for fk in await db.foreign_keys_for_table(table)
diff --git a/tests/test_html.py b/tests/test_html.py
index 85155a8e..81d128c9 100644
--- a/tests/test_html.py
+++ b/tests/test_html.py
@@ -344,21 +344,37 @@ def test_sort_links(app_client):
     ]
     assert [
         {
-            "attrs": {"class": ["col-Link"], "data-column": "Link", "scope": "col"},
+            "attrs": {
+                "class": ["col-Link"],
+                "data-column": "Link",
+                "data-is-pk": "0",
+                "scope": "col",
+            },
             "a_href": None,
         },
         {
-            "attrs": {"class": ["col-pk1"], "data-column": "pk1", "scope": "col"},
+            "attrs": {
+                "class": ["col-pk1"],
+                "data-column": "pk1",
+                "data-is-pk": "1",
+                "scope": "col",
+            },
             "a_href": None,
         },
         {
-            "attrs": {"class": ["col-pk2"], "data-column": "pk2", "scope": "col"},
+            "attrs": {
+                "class": ["col-pk2"],
+                "data-column": "pk2",
+                "data-is-pk": "1",
+                "scope": "col",
+            },
             "a_href": None,
         },
         {
             "attrs": {
                 "class": ["col-content"],
                 "data-column": "content",
+                "data-is-pk": "0",
                 "scope": "col",
             },
             "a_href": None,
@@ -367,6 +383,7 @@ def test_sort_links(app_client):
             "attrs": {
                 "class": ["col-sortable"],
                 "data-column": "sortable",
+                "data-is-pk": "0",
                 "scope": "col",
             },
             "a_href": "sortable?_sort_desc=sortable",
@@ -375,6 +392,7 @@ def test_sort_links(app_client):
             "attrs": {
                 "class": ["col-sortable_with_nulls"],
                 "data-column": "sortable_with_nulls",
+                "data-is-pk": "0",
                 "scope": "col",
             },
             "a_href": "sortable?_sort=sortable_with_nulls",
@@ -383,12 +401,18 @@ def test_sort_links(app_client):
             "attrs": {
                 "class": ["col-sortable_with_nulls_2"],
                 "data-column": "sortable_with_nulls_2",
+                "data-is-pk": "0",
                 "scope": "col",
             },
             "a_href": "sortable?_sort=sortable_with_nulls_2",
         },
         {
-            "attrs": {"class": ["col-text"], "data-column": "text", "scope": "col"},
+            "attrs": {
+                "class": ["col-text"],
+                "data-column": "text",
+                "data-is-pk": "0",
+                "scope": "col",
+            },
             "a_href": "sortable?_sort=text",
         },
     ] == attrs_and_link_attrs