[PATCH v2 4/5] patch-detail: add functionality for comment status updates
Daniel Axtens
dja at axtens.net
Fri Aug 6 12:56:37 AEST 2021
Raxel Gutierrez <raxel at google.com> writes:
> Use new comment detail REST API endpoint to update the addressed field
> value when "Addressed" or "Unaddressed" buttons are clicked. After, the
> request is made, the appearance of the comment status label and buttons
> are toggled appropriately.
As a general note, if you're able to split the code movement parts out
from the new code parts, I am happy to merge the refactoring earlier.
That will also make it easier for me to do reviews.
Overall this patch (and by extension the series) seems to work in a
reasonable way.
I go back and forth on whether this needs to sit behind
a project gate flag; on one hand it's possibly confusing for a submitter
to have state tracked here that the maintainer doesn't care about. On
the other hand, it isn't a massive UI change and maybe there's no real
harm in leaving a feature around that people don't use. Let's stick with
having it on everywhere for now.
> Signed-off-by: Raxel Gutierrez <raxel at google.com>
> ---
> htdocs/css/style.css | 15 ++++-
> htdocs/js/submission.js | 52 ++++++++++++++++
> patchwork/templates/patchwork/submission.html | 61 ++++++-------------
> templates/base.html | 2 +-
> 4 files changed, 85 insertions(+), 45 deletions(-)
> create mode 100644 htdocs/js/submission.js
>
> diff --git a/htdocs/css/style.css b/htdocs/css/style.css
> index ff34ff5..2a5c81f 100644
> --- a/htdocs/css/style.css
> +++ b/htdocs/css/style.css
> @@ -27,6 +27,10 @@ pre {
> top: 17em;
> }
>
> +.hidden {
> + visibility: hidden;
> +}
> +
> /* Bootstrap overrides */
>
> .navbar-inverse .navbar-brand > a {
> @@ -306,7 +310,7 @@ table.patchmeta tr th, table.patchmeta tr td {
> font-family: "DejaVu Sans Mono", fixed;
> }
>
> -#comment-status-bar {
> +div[id^="comment-status-bar-"] {
> display: flex;
> flex-wrap: wrap;
> align-items: center;
> @@ -316,7 +320,7 @@ table.patchmeta tr th, table.patchmeta tr td {
> margin: 0px 8px;
> }
>
> -#comment-action-addressed, #comment-action-unaddressed {
> +button[id^=comment-action] {
> background-color: var(--light-color);
> border-radius: 4px;
> }
> @@ -329,11 +333,16 @@ table.patchmeta tr th, table.patchmeta tr td {
> border-color: var(--warning-color);
> }
>
> -#comment-action-addressed:hover, #comment-action-unaddressed:hover {
> +#comment-action-addressed:hover {
> background-color: var(--success-color);
> color: var(--light-color);
> }
>
> +#comment-action-unaddressed:hover {
> + background-color: var(--warning-color);
> + color: var(--light-color);
> +}
> +
> .quote {
> color: #007f00;
> }
> diff --git a/htdocs/js/submission.js b/htdocs/js/submission.js
> new file mode 100644
> index 0000000..8894890
> --- /dev/null
> +++ b/htdocs/js/submission.js
> @@ -0,0 +1,52 @@
> +import { updateProperty } from "./rest.js";
> +
> +$( document ).ready(function() {
> + function toggleDiv(link_id, headers_id, label_show, label_hide) {
> + const link = document.getElementById(link_id)
> + const headers = document.getElementById(headers_id)
> +
> + const hidden = headers.style['display'] == 'none';
> +
> + if (hidden) {
> + link.innerHTML = label_hide || 'hide';
> + headers.style['display'] = 'block';
> + } else {
> + link.innerHTML = label_show || 'show';
> + headers.style['display'] = 'none';
> + }
> + }
> +
> + $("button[id^='comment-action']").click((event) => {
> + const patchId = document.getElementById("patch").dataset.patchId;
> + const commentId = event.target.parentElement.dataset.commentId;
> + const url = "/api/patches/" + patchId + "/comments/" + commentId + "/";
> + const data = {'addressed': event.target.value} ;
> + const updateMessage = {
> + 'none': "No comments updated",
> + 'some': "1 comment updated",
> + };
> + updateProperty(url, data, updateMessage);
> + $("div[id^='comment-status-bar-'][data-comment-id='"+commentId+"']").toggleClass("hidden");
> + });
> +
> + // Click listener to show/hide headers
> + document.getElementById("toggle-patch-headers").addEventListener("click", function() {
> + toggleDiv("toggle-patch-headers", "patch-headers");
> + });
> +
> + // Click listener to expand/collapse series
> + document.getElementById("toggle-patch-series").addEventListener("click", function() {
> + toggleDiv("toggle-patch-series", "patch-series", "expand", "collapse");
> + });
> +
> + // Click listener to show/hide related patches
> + document.getElementById("toggle-related").addEventListener("click", function() {
> + toggleDiv("toggle-related", "related");
> + });
> +
> + // Click listener to show/hide related patches from different projects
> + document.getElementById("toggle-related-outside").addEventListener("click", function() {
> + toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
> + });
> +
> +});
This code threw a bunch of JS errors for me because I looked at a patch
that didn't have any related patches. I ended up doing the following:
diff --git a/htdocs/js/submission.js b/htdocs/js/submission.js
index 88948902ec56..56fbac64269e 100644
--- a/htdocs/js/submission.js
+++ b/htdocs/js/submission.js
@@ -40,13 +40,18 @@ $( document ).ready(function() {
});
// Click listener to show/hide related patches
- document.getElementById("toggle-related").addEventListener("click", function() {
- toggleDiv("toggle-related", "related");
- });
-
+ var related = document.getElementById("toggle-related");
+ if (related) {
+ related.addEventListener("click", function() {
+ toggleDiv("toggle-related", "related");
+ });
+ }
// Click listener to show/hide related patches from different projects
- document.getElementById("toggle-related-outside").addEventListener("click", function() {
- toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
- });
+ var related_outside = document.getElementById("toggle-related-outside");
+ if (related_outside) {
+ related_outside.addEventListener("click", function() {
+ toggleDiv("toggle-related-outside", "related-outside", "show from other projects");
+ });
+ }
});
Apologies for the shocking JS, I'm not a front-end person at all.
> \ No newline at end of file
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 4109442..e3a8909 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -5,29 +5,15 @@
> {% load syntax %}
> {% load person %}
> {% load patch %}
> +{% load static %}
> +
> +{% block headers %}
> + <script type="module" src="{% static "js/submission.js" %}"></script>
> +{% endblock %}
>
> {% block title %}{{submission.name}}{% endblock %}
>
> {% block body %}
> -<script>
> -function toggle_div(link_id, headers_id, label_show, label_hide)
> -{
> - var link = document.getElementById(link_id)
> - var headers = document.getElementById(headers_id)
> -
> - var hidden = headers.style['display'] == 'none';
> -
> - if (hidden) {
> - link.innerHTML = label_hide || 'hide';
> - headers.style['display'] = 'block';
> - } else {
> - link.innerHTML = label_show || 'show';
> - headers.style['display'] = 'none';
> - }
> -
> -}
> -</script>
> -
> <div>
> {% include "patchwork/partials/download-buttons.html" %}
> <h1>{{ submission.name }}</h1>
> @@ -63,10 +49,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> <tr>
> <th>Headers</th>
> <td>
> - <button
> - id="toggle-patch-headers"
> - onclick="javascript:toggle_div('toggle-patch-headers', 'patch-headers')"
> - >show</button>
> + <button id="toggle-patch-headers">show</button>
> <div id="patch-headers" class="patch-headers" style="display:none;">
> <pre>{{submission.headers}}</pre>
> </div>
> @@ -79,10 +62,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ submission.series.id }}">
> {{ submission.series.name }}
> </a>
> - <button
> - id="toggle-patch-series"
> - onclick="javascript:toggle_div('toggle-patch-series', 'patch-series', 'expand', 'collapse')"
> - >expand</button>
> + <button id="toggle-patch-series">expand</button>
> <div id="patch-series" class="submission-list" style="display:none;">
> <ul>
> {% with submission.series.cover_letter as cover %}
> @@ -118,10 +98,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> <tr>
> <th>Related</th>
> <td>
> - <button
> - id="toggle-related"
> - onclick="javascript:toggle_div('toggle-related', 'related')"
> - >show</button>
> + <button id="toggle-related">show</button>
> <div id="related" class="submission-list" style="display:none;">
> <ul>
> {% for sibling in related_same_project %}
> @@ -134,10 +111,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> </li>
> {% endfor %}
> {% if related_different_project %}
> - <button
> - id="toggle-related-outside"
> - onclick="javascript:toggle_div('toggle-related-outside', 'related-outside', 'show from other projects')"
> - >show from other projects</button>
> + <button id="toggle-related-outside">show from other projects</button>
> <div id="related-outside" class="submission-list" style="display:none;">
> {% for sibling in related_outside %}
> <li>
> @@ -306,32 +280,37 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> <a href="{% url 'comment-redirect' comment_id=item.id %}">#{{ forloop.counter }}</a>
> </span>
> {% if item.addressed %}
> - <div id="comment-status-bar">
> + <div id="comment-status-bar-addressed" data-comment-id={{item.id}}>
> + {% else %}
> + <div id="comment-status-bar-addressed" class="hidden" data-comment-id={{item.id}}>
> + {% endif %}
> <div id="comment-status-label" class="text-success mx-3">
> <span id="comment-status-icon" class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
> Addressed
> </div>
> {% if editable %}
> - <button id="comment-action-unaddressed" class="text-warning">
> + <button id="comment-action-unaddressed" class="text-warning" value="false">
> <span id="comment-action-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
> Unaddressed
> </button>
> {% endif %}
> </div>
> + {% if item.addressed %}
> + <div id="comment-status-bar-unaddressed" class="hidden" data-comment-id={{item.id}}>
> {% else %}
> - <div id="comment-status-bar">
> + <div id="comment-status-bar-unaddressed" data-comment-id={{item.id}}>
> + {% endif %}
> <div id="comment-status-label" class="text-warning mx-3">
> <span id="comment-status-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span>
> Unaddressed
> </div>
> {% if editable %}
> - <button id="comment-action-addressed" class="text-success">
> + <button id="comment-action-addressed" class="text-success" value="true">
> <span id="comment-action-icon" class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span>
> Addressed
> </button>
> {% endif %}
> </div>
> - {% endif %}
> </div>
> <pre class="content">
> {{ item|commentsyntax }}
> @@ -344,7 +323,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide)
> {% include "patchwork/partials/download-buttons.html" %}
> <h2>Patch</h2>
> </div>
> -<div id="patch" class="patch">
> +<div id="patch" data-patch-id={{submission.id}} class="patch">
> <pre class="content">
> {{ submission|patchsyntax }}
> </pre>
> diff --git a/templates/base.html b/templates/base.html
> index 8accb4c..a95a11b 100644
> --- a/templates/base.html
> +++ b/templates/base.html
> @@ -113,7 +113,7 @@
> {% endfor %}
> </div>
> {% endif %}
> - <div class="container-fluid">
> + <div id="main-content" class="container-fluid">
> {% block body %}
> {% endblock %}
> </div>
> --
> 2.32.0.554.ge1b32706d8-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list