Raxel Gutierrez <ra...@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 <ra...@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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork