Message ID | 20210728181718.3613124-5-raxel@google.com |
---|---|
State | Changes Requested |
Headers | show |
Series | patch-detail: add unaddressed/addressed status to patch comments | expand |
Related | show |
Raxel Gutierrez <raxel@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@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
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"); + }); + +}); \ 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>
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. Signed-off-by: Raxel Gutierrez <raxel@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