diff mbox series

[2/5] patch-list: refactored html and patch list forms

Message ID 20210719233428.2045872-3-raxel@google.com
State Superseded
Headers show
Series patch-list: improve usability of list action bar | expand

Commit Message

Raxel Gutierrez July 19, 2021, 11:34 p.m. UTC
Refactored the patch list frontend code to make the forms more healthy
and readable. Also, separated script tags into a separate js file which
is better for keeping things modular and maintainable. Refer to cover
letter for other conventions that I suggest for frontend code like
naming for HTML attributes / CSS selectors.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 htdocs/README.rst                             |   7 +
 htdocs/js/patch-list.js                       |  12 ++
 .../patchwork/partials/patch-list.html        | 170 +++++++-----------
 patchwork/views/__init__.py                   |   2 +
 4 files changed, 83 insertions(+), 108 deletions(-)
 create mode 100644 htdocs/js/patch-list.js

Comments

Jonathan Nieder July 20, 2021, 1:47 a.m. UTC | #1
Hi,

Raxel Gutierrez wrote:

> Refactored the patch list frontend code to make the forms more healthy
> and readable. Also, separated script tags into a separate js file which
> is better for keeping things modular and maintainable.

Tiny nit: patchwork's commit messages tend to use the imperative mood,
as though ordering the codebase to change.  In other words, you can
think of a commit message as kind of like a bug report --- it
describes a change you would like the project to make and why.

In this example, I think the idea is something like

	Clean up the patch-list page by <explanation>.  This allows <description
	of benefit>.  No user-visible change intended.

	Also move patch list related js code to a new patch-list.js file, to
	make the javascript easy to read and edit in one place.  This makes
	automatic code formatting easier, makes it more straightforward to
	measure test coverage and discover opportunities for refactoring, and
	simplifies a possible future migration to typescript if the project
	chooses to go in that direction.

>                                                        Refer to cover
> letter for other conventions that I suggest for frontend code like
> naming for HTML attributes / CSS selectors.

The commit message, unlike the cover letter, becomes part of the
history I can see in the patchwork repository with "git log" (e.g.,
when trying to understand a line I am investigating using "git
blame"); as a result, it's helpful for the commit messages to be
mostly self-contained.  Are there particular conventions that are
relevant for this patch that should be included here?  Or does this
suggest that it might make sense to add a file about frontend coding
style to docs/development/?

Thanks and hope that helps,
Jonathan
diff mbox series

Patch

diff --git a/htdocs/README.rst b/htdocs/README.rst
index dfa7ca8..4441bf3 100644
--- a/htdocs/README.rst
+++ b/htdocs/README.rst
@@ -131,6 +131,13 @@  js
   :GitHub: https://github.com/js-cookie/js-cookie/
   :Version: 2.2.1
 
+``patch-list.js.``
+
+  Utility functions for patch list manipulation (inline dropdown changes,
+  etc.)
+
+  Part of Patchwork.
+
 ``selectize.min.js``
 
   Selectize is the hybrid of a ``textbox`` and ``<select>`` box. It's jQuery
diff --git a/htdocs/js/patch-list.js b/htdocs/js/patch-list.js
new file mode 100644
index 0000000..8c7640f
--- /dev/null
+++ b/htdocs/js/patch-list.js
@@ -0,0 +1,12 @@ 
+$( document ).ready(function() {
+    $("#patchlist").stickyTableHeaders();
+
+    $("#check-all").change(function(e) {
+        if(this.checked) {
+            $("#patchlist > tbody").checkboxes("check");
+        } else {
+            $("#patchlist > tbody").checkboxes("uncheck");
+        }
+        e.preventDefault();
+    });
+});
\ No newline at end of file
diff --git a/patchwork/templates/patchwork/partials/patch-list.html b/patchwork/templates/patchwork/partials/patch-list.html
index 02d6dff..629922c 100644
--- a/patchwork/templates/patchwork/partials/patch-list.html
+++ b/patchwork/templates/patchwork/partials/patch-list.html
@@ -4,9 +4,11 @@ 
 {% load project %}
 {% load static %}
 
-{% include "patchwork/partials/filters.html" %}
+{% block headers %}
+  <script src="{% static "js/patch-list.js" %}"></script>
+{% endblock %}
 
-{% include "patchwork/partials/pagination.html" %}
+{% include "patchwork/partials/filters.html" %}
 
 {% if order.editable %}
 <table class="patchlist">
@@ -35,25 +37,57 @@ 
 </div>
 {% endif %}
 
-<script type="text/javascript">
-$(document).ready(function() {
-    $('#patchlist').stickyTableHeaders();
-
-    $('#check-all').change(function(e) {
-        if(this.checked) {
-            $('#patchlist > tbody').checkboxes('check');
-        } else {
-            $('#patchlist > tbody').checkboxes('uncheck');
-        }
-        e.preventDefault();
-    });
-});
-</script>
-
-<form method="post">
+<form id="patch-list-form" method="post">
 {% csrf_token %}
 <input type="hidden" name="form" value="patchlistform"/>
 <input type="hidden" name="project" value="{{project.id}}"/>
+<div class="patch-list-actions">
+  <div class="patchforms" id="patchforms">
+    {% if patchform %}
+      <div id="patchform-properties" class="patchform">
+        <div id="patchform-state">
+          {{ patchform.state.errors }}
+          {{ patchform.state }}
+        </div>
+        <div id="patchform-delegate">
+          {{ patchform.delegate.errors }}
+          {{ patchform.delegate }}
+        </div>
+        <div id="patchform-archive">
+          {{ patchform.archived.errors }}
+          {{ patchform.archived.label_tag }} {{ patchform.archived }}
+        </div>
+        <button class="patchform-submit btn btn-primary" name="action" value="{{patchform.action}}">Update</button>
+      </div>
+    {% endif %}
+    {% if user.is_authenticated %}
+      <div id="patchform-bundle" class="patchform">
+        <div id="create-bundle">
+          <input class="create-bundle" type="text" name="bundle_name" placeholder="Bundle name"/>
+          <input class="patchform-submit btn btn-primary" name="action" value="Create" type="submit"/>
+        </div>
+        {% if bundles %}
+        <div id="add-to-bundle">
+          <select class="add-bundle" name="bundle_id">
+            <option value="" selected>Add to bundle</option>
+            {% for bundle in bundles %}
+             <option value="{{bundle.id}}">{{bundle.name}}</option>
+            {% endfor %}
+          </select>
+          <input class="patchform-submit btn btn-primary" name="action" value="Add" type="submit"/>
+        </div>
+        {% endif %}
+        {% if bundle %}
+        <div id="remove-bundle">
+          <input type="hidden" name="removed_bundle_id" value="{{bundle.id}}"/>
+          <button class="patchform-submit btn btn-primary" name="action" value="Remove">Remove from bundle</button>
+        </div>
+        {% endif %}
+      </div>
+    {% endif %}
+  </div>
+  {% include "patchwork/partials/pagination.html" %}
+</div>
 <table id="patchlist" class="table table-hover table-extra-condensed table-striped pw-list"
        data-toggle="checkboxes" data-range="true">
  <thead>
@@ -174,9 +208,9 @@  $(document).ready(function() {
 
  <tbody>
  {% for patch in page.object_list %}
-  <tr id="patch_row:{{patch.id}}">
+  <tr id="patch_row:{{patch.id}}" data-patch-id="{{patch.id}}">
    {% if user.is_authenticated %}
-   <td style="text-align: center;">
+   <td id="select-patch" style="text-align: center;">
     <input type="checkbox" name="patch_id:{{patch.id}}"/>
    </td>
    {% endif %}
@@ -188,24 +222,24 @@  $(document).ready(function() {
     </button>
    </td>
    {% endif %}
-   <td>
+   <td id="patch-name">
     <a href="{% url 'patch-detail' project_id=project.linkname msgid=patch.url_msgid %}">
      {{ patch.name|default:"[no subject]"|truncatechars:100 }}
     </a>
    </td>
-   <td>
+   <td id="patch-series">
     {% if patch.series %}
     <a href="?series={{patch.series.id}}">
      {{ patch.series|truncatechars:100 }}
     </a>
     {% endif %}
    </td>
-   <td class="text-nowrap">{{ patch|patch_tags }}</td>
-   <td class="text-nowrap">{{ patch|patch_checks }}</td>
-   <td class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
-   <td>{{ patch.submitter|personify:project }}</td>
-   <td>{{ patch.delegate.username }}</td>
-   <td>{{ patch.state }}</td>
+   <td id="patch-tags" class="text-nowrap">{{ patch|patch_tags }}</td>
+   <td id="patch-checks" class="text-nowrap">{{ patch|patch_checks }}</td>
+   <td id="patch-date" class="text-nowrap">{{ patch.date|date:"Y-m-d" }}</td>
+   <td id="patch-submitter">{{ patch.submitter|personify:project }}</td>
+   <td id="patch-delegate">{{ patch.delegate.username }}</td>
+   <td id="patch-state">{{ patch.state }}</td>
   </tr>
  {% empty %}
   <tr>
@@ -217,86 +251,6 @@  $(document).ready(function() {
 
 {% if page.paginator.count %}
 {% include "patchwork/partials/pagination.html" %}
-
-<div class="patchforms" id="patchforms">
-
-{% if patchform %}
- <div class="patchform patchform-properties">
-  <h3>Properties</h3>
-    <table class="form">
-     <tr>
-      <th>Change state:</th>
-      <td>
-       {{ patchform.state }}
-       {{ patchform.state.errors }}
-      </td>
-     </tr>
-     <tr>
-      <th>Delegate to:</th>
-      <td>
-       {{ patchform.delegate }}
-       {{ patchform.delegate.errors }}
-      </td>
-     </tr>
-     <tr>
-      <th>Archive:</th>
-      <td>
-       {{ patchform.archived }}
-       {{ patchform.archived.errors }}
-      </td>
-     </tr>
-     <tr>
-      <td></td>
-      <td>
-       <input type="submit" name="action" value="{{patchform.action}}"/>
-      </td>
-     </tr>
-    </table>
- </div>
-
-{% endif %}
-
-{% if user.is_authenticated %}
- <div class="patchform patchform-bundle">
-  <h3>Bundling</h3>
-   <table class="form">
-    <tr>
-     <td>Create bundle:</td>
-     <td>
-      <input type="text" name="bundle_name"/>
-      <input name="action" value="Create" type="submit"/>
-      </td>
-    </tr>
-  {% if bundles %}
-    <tr>
-     <td>Add to bundle:</td>
-     <td>
-       <select name="bundle_id">
-        {% for bundle in bundles %}
-         <option value="{{bundle.id}}">{{bundle.name}}</option>
-        {% endfor %}
-        </select>
-       <input name="action" value="Add" type="submit"/>
-     </td>
-    </tr>
-  {% endif %}
-  {% if bundle %}
-   <tr>
-     <td>Remove from bundle:</td>
-     <td>
-       <input type="hidden" name="removed_bundle_id" value="{{bundle.id}}"/>
-       <input name="action" value="Remove" type="submit"/>
-     </td>
-    </tr>
-  {% endif %}
-  </table>
- </div>
-{% endif %}
-
- <div style="clear: both;">
- </div>
-</div>
-
 {% endif %}
 
 </form>
diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
index 3efe90c..6c30f0b 100644
--- a/patchwork/views/__init__.py
+++ b/patchwork/views/__init__.py
@@ -129,6 +129,8 @@  def set_bundle(request, project, action, data, patches, context):
         bundle.save()
         messages.success(request, "Bundle %s created" % bundle.name)
     elif action == 'add':
+        if not data['bundle_id']:
+            return ['No bundle was selected']
         bundle = get_object_or_404(Bundle, id=data['bundle_id'])
     elif action == 'remove':
         bundle = get_object_or_404(Bundle, id=data['removed_bundle_id'])