Message ID | 20210728181718.3613124-4-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: > Add new label to show the status of whether a comment is addressed and a > button to change the status of the comment. Only users that can edit > the patch (submitter, delegate, project maintainers) can change the > status of a comment. Clean up submission.html to have hyphen delimited > id and class selector names as well. Before [1] and after [2] images > for reference. > So another thing you do in this patch is change the show related and show headers from links to buttons. Please could you split this up. I think there are 3 things going on, each of which should be in its own patch: - rename things/refactor - change show related/show header links to buttons - add UI elements for comment addressed status But if there's a different, more logical or simpler split feel free to do that. Skimming the diff I don't have many review comments. I'll do a deeper dive when it's split. > [1] https://imgur.com/NDfcPJy > [2] https://imgur.com/kIhohED > > Signed-off-by: Raxel Gutierrez <raxel@google.com> > --- > htdocs/css/style.css | 46 +++++- > patchwork/templates/patchwork/submission.html | 146 +++++++++++------- > patchwork/views/patch.py | 1 + > 3 files changed, 134 insertions(+), 59 deletions(-) > > diff --git a/htdocs/css/style.css b/htdocs/css/style.css > index 243caa0..ff34ff5 100644 > --- a/htdocs/css/style.css > +++ b/htdocs/css/style.css > @@ -1,3 +1,9 @@ > +:root { > + --light-color: #F7F7F7; > + --warning-color: #f0ad4e; > + --success-color: #5cb85c; > +} > + > h2 { > font-size: 25px; > margin: 18px 0 18px 0; > @@ -192,7 +198,7 @@ table.patchmeta tr th, table.patchmeta tr td { > vertical-align: top; > } > > -.submissionlist ul { > +.submission-list ul { > list-style-type: none; > padding: 0; > margin: 0; > @@ -277,12 +283,18 @@ table.patchmeta tr th, table.patchmeta tr td { > color: #f7977a; > } > > -.comment .meta { > +.patch-message .meta { > + display: flex; > + align-items: center; > background: #f0f0f0; > padding: 0.3em 0.5em; > } > > -.comment .content { > +.patch-message .message-date { > + margin-left: 8px; > +} > + > +.patch-message .content { > border: 0; > } > > @@ -294,6 +306,34 @@ table.patchmeta tr th, table.patchmeta tr td { > font-family: "DejaVu Sans Mono", fixed; > } > > +#comment-status-bar { > + display: flex; > + flex-wrap: wrap; > + align-items: center; > +} > + > +#comment-status-label { > + margin: 0px 8px; > +} > + > +#comment-action-addressed, #comment-action-unaddressed { > + background-color: var(--light-color); > + border-radius: 4px; > +} > + > +#comment-action-addressed { > + border-color: var(--success-color); > +} > + > +#comment-action-unaddressed { > + border-color: var(--warning-color); > +} > + > +#comment-action-addressed:hover, #comment-action-unaddressed:hover { > + background-color: var(--success-color); > + color: var(--light-color); > +} > + > .quote { > color: #007f00; > } > diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html > index 978559b..4109442 100644 > --- a/patchwork/templates/patchwork/submission.html > +++ b/patchwork/templates/patchwork/submission.html > @@ -1,3 +1,4 @@ > + You probably don't need the extra newline here? > {% extends "base.html" %} > > {% load humanize %} > @@ -32,7 +33,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide) > <h1>{{ submission.name }}</h1> > </div> > > -<table class="patchmeta"> > +<table class="patch-meta"> > <tr> > <th>Message ID</th> > {% if submission.list_archive_url %} > @@ -61,12 +62,14 @@ function toggle_div(link_id, headers_id, label_show, label_hide) > {% endif %} > <tr> > <th>Headers</th> > - <td><a id="togglepatchheaders" > - href="javascript:toggle_div('togglepatchheaders', 'patchheaders')" > - >show</a> > - <div id="patchheaders" class="patchheaders" style="display:none;"> > - <pre>{{submission.headers}}</pre> > - </div> > + <td> > + <button > + id="toggle-patch-headers" > + onclick="javascript:toggle_div('toggle-patch-headers', 'patch-headers')" > + >show</button> > + <div id="patch-headers" class="patch-headers" style="display:none;"> > + <pre>{{submission.headers}}</pre> > + </div> > </td> > </tr> > {% if submission.series %} > @@ -75,11 +78,12 @@ function toggle_div(link_id, headers_id, label_show, label_hide) > <td> > <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ submission.series.id }}"> > {{ submission.series.name }} > - </a> | > - <a id="togglepatchseries" > - href="javascript:toggle_div('togglepatchseries', 'patchseries', 'expand', 'collapse')" > - >expand</a> > - <div id="patchseries" class="submissionlist" style="display:none;"> > + </a> > + <button > + id="toggle-patch-series" > + onclick="javascript:toggle_div('toggle-patch-series', 'patch-series', 'expand', 'collapse')" > + >expand</button> > + <div id="patch-series" class="submission-list" style="display:none;"> > <ul> > {% with submission.series.cover_letter as cover %} > <li> > @@ -114,36 +118,38 @@ function toggle_div(link_id, headers_id, label_show, label_hide) > <tr> > <th>Related</th> > <td> > - <a id="togglerelated" > - href="javascript:toggle_div('togglerelated', 'related')" > - >show</a> > - <div id="related" class="submissionlist" style="display:none;"> > - <ul> > - {% for sibling in related_same_project %} > - <li> > - {% if sibling.id != submission.id %} > - <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}"> > - {{ sibling.name|default:"[no subject]"|truncatechars:100 }} > - </a> > - {% endif %} > - </li> > - {% endfor %} > - {% if related_different_project %} > - <a id="togglerelatedoutside" > - href="javascript:toggle_div('togglerelatedoutside', 'relatedoutside', 'show from other projects')" > - >show from other projects</a> > - <div id="relatedoutside" class="submissionlist" style="display:none;"> > - {% for sibling in related_outside %} > + <button > + id="toggle-related" > + onclick="javascript:toggle_div('toggle-related', 'related')" > + >show</button> > + <div id="related" class="submission-list" style="display:none;"> > + <ul> > + {% for sibling in related_same_project %} > <li> > - <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}"> > + {% if sibling.id != submission.id %} > + <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}"> > {{ sibling.name|default:"[no subject]"|truncatechars:100 }} > - </a> (in {{ sibling.project }}) > + </a> > + {% endif %} > </li> > - {% endfor %} > - </div> > - {% endif %} > - </ul> > - </div> > + {% 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> > + <div id="related-outside" class="submission-list" style="display:none;"> > + {% for sibling in related_outside %} > + <li> > + <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}"> > + {{ sibling.name|default:"[no subject]"|truncatechars:100 }} > + </a> (in {{ sibling.project }}) > + </li> > + {% endfor %} > + </div> > + {% endif %} > + </ul> > + </div> > </td> > </tr> > {% endif %} > @@ -277,14 +283,14 @@ function toggle_div(link_id, headers_id, label_show, label_hide) > {% else %} > <h2>Message</h2> > {% endif %} > -<div class="comment"> > -<div class="meta"> > - <span>{{ submission.submitter|personify:project }}</span> > - <span class="pull-right">{{ submission.date }} UTC</span> > -</div> > -<pre class="content"> > -{{ submission|commentsyntax }} > -</pre> > +<div class="patch-message"> > + <div class="meta"> > + <span>{{ submission.submitter|personify:project }}</span> > + <span class="message-date">{{ submission.date }} UTC</span> > + </div> > + <pre class="content"> > + {{ submission|commentsyntax }} > + </pre> > </div> > > {% for item in comments %} > @@ -293,15 +299,43 @@ function toggle_div(link_id, headers_id, label_show, label_hide) > {% endif %} > > <a name="{{ item.id }}"></a> > -<div class="comment"> > -<div class="meta"> > - <span>{{ item.submitter|personify:project }}</span> > - <span class="pull-right">{{ item.date }} UTC | <a href="{% url 'comment-redirect' comment_id=item.id %}" > - >#{{ forloop.counter }}</a></span> > -</div> > -<pre class="content"> > -{{ item|commentsyntax }} > -</pre> > +<div class="patch-message"> > + <div class="meta"> > + {{ item.submitter|personify:project }} > + <span class="message-date">{{ item.date }} UTC | > + <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-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"> > + <span id="comment-action-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span> > + Unaddressed > + </button> > + {% endif %} > + </div> > + {% else %} > + <div id="comment-status-bar"> > + <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"> > + <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 }} > + </pre> > </div> > {% endfor %} > > diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py > index 3e6874a..ac9cb44 100644 > --- a/patchwork/views/patch.py > +++ b/patchwork/views/patch.py > @@ -128,6 +128,7 @@ def patch_detail(request, project_id, msgid): > patch.check_set.all().select_related('user'), > ) > context['submission'] = patch > + context['editable'] = editable > context['patchform'] = form > context['createbundleform'] = createbundleform > context['project'] = patch.project > -- > 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 243caa0..ff34ff5 100644 --- a/htdocs/css/style.css +++ b/htdocs/css/style.css @@ -1,3 +1,9 @@ +:root { + --light-color: #F7F7F7; + --warning-color: #f0ad4e; + --success-color: #5cb85c; +} + h2 { font-size: 25px; margin: 18px 0 18px 0; @@ -192,7 +198,7 @@ table.patchmeta tr th, table.patchmeta tr td { vertical-align: top; } -.submissionlist ul { +.submission-list ul { list-style-type: none; padding: 0; margin: 0; @@ -277,12 +283,18 @@ table.patchmeta tr th, table.patchmeta tr td { color: #f7977a; } -.comment .meta { +.patch-message .meta { + display: flex; + align-items: center; background: #f0f0f0; padding: 0.3em 0.5em; } -.comment .content { +.patch-message .message-date { + margin-left: 8px; +} + +.patch-message .content { border: 0; } @@ -294,6 +306,34 @@ table.patchmeta tr th, table.patchmeta tr td { font-family: "DejaVu Sans Mono", fixed; } +#comment-status-bar { + display: flex; + flex-wrap: wrap; + align-items: center; +} + +#comment-status-label { + margin: 0px 8px; +} + +#comment-action-addressed, #comment-action-unaddressed { + background-color: var(--light-color); + border-radius: 4px; +} + +#comment-action-addressed { + border-color: var(--success-color); +} + +#comment-action-unaddressed { + border-color: var(--warning-color); +} + +#comment-action-addressed:hover, #comment-action-unaddressed:hover { + background-color: var(--success-color); + color: var(--light-color); +} + .quote { color: #007f00; } diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html index 978559b..4109442 100644 --- a/patchwork/templates/patchwork/submission.html +++ b/patchwork/templates/patchwork/submission.html @@ -1,3 +1,4 @@ + {% extends "base.html" %} {% load humanize %} @@ -32,7 +33,7 @@ function toggle_div(link_id, headers_id, label_show, label_hide) <h1>{{ submission.name }}</h1> </div> -<table class="patchmeta"> +<table class="patch-meta"> <tr> <th>Message ID</th> {% if submission.list_archive_url %} @@ -61,12 +62,14 @@ function toggle_div(link_id, headers_id, label_show, label_hide) {% endif %} <tr> <th>Headers</th> - <td><a id="togglepatchheaders" - href="javascript:toggle_div('togglepatchheaders', 'patchheaders')" - >show</a> - <div id="patchheaders" class="patchheaders" style="display:none;"> - <pre>{{submission.headers}}</pre> - </div> + <td> + <button + id="toggle-patch-headers" + onclick="javascript:toggle_div('toggle-patch-headers', 'patch-headers')" + >show</button> + <div id="patch-headers" class="patch-headers" style="display:none;"> + <pre>{{submission.headers}}</pre> + </div> </td> </tr> {% if submission.series %} @@ -75,11 +78,12 @@ function toggle_div(link_id, headers_id, label_show, label_hide) <td> <a href="{% url 'patch-list' project_id=project.linkname %}?series={{ submission.series.id }}"> {{ submission.series.name }} - </a> | - <a id="togglepatchseries" - href="javascript:toggle_div('togglepatchseries', 'patchseries', 'expand', 'collapse')" - >expand</a> - <div id="patchseries" class="submissionlist" style="display:none;"> + </a> + <button + id="toggle-patch-series" + onclick="javascript:toggle_div('toggle-patch-series', 'patch-series', 'expand', 'collapse')" + >expand</button> + <div id="patch-series" class="submission-list" style="display:none;"> <ul> {% with submission.series.cover_letter as cover %} <li> @@ -114,36 +118,38 @@ function toggle_div(link_id, headers_id, label_show, label_hide) <tr> <th>Related</th> <td> - <a id="togglerelated" - href="javascript:toggle_div('togglerelated', 'related')" - >show</a> - <div id="related" class="submissionlist" style="display:none;"> - <ul> - {% for sibling in related_same_project %} - <li> - {% if sibling.id != submission.id %} - <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}"> - {{ sibling.name|default:"[no subject]"|truncatechars:100 }} - </a> - {% endif %} - </li> - {% endfor %} - {% if related_different_project %} - <a id="togglerelatedoutside" - href="javascript:toggle_div('togglerelatedoutside', 'relatedoutside', 'show from other projects')" - >show from other projects</a> - <div id="relatedoutside" class="submissionlist" style="display:none;"> - {% for sibling in related_outside %} + <button + id="toggle-related" + onclick="javascript:toggle_div('toggle-related', 'related')" + >show</button> + <div id="related" class="submission-list" style="display:none;"> + <ul> + {% for sibling in related_same_project %} <li> - <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}"> + {% if sibling.id != submission.id %} + <a href="{% url 'patch-detail' project_id=project.linkname msgid=sibling.url_msgid %}"> {{ sibling.name|default:"[no subject]"|truncatechars:100 }} - </a> (in {{ sibling.project }}) + </a> + {% endif %} </li> - {% endfor %} - </div> - {% endif %} - </ul> - </div> + {% 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> + <div id="related-outside" class="submission-list" style="display:none;"> + {% for sibling in related_outside %} + <li> + <a href="{% url 'patch-detail' project_id=sibling.project.linkname msgid=sibling.url_msgid %}"> + {{ sibling.name|default:"[no subject]"|truncatechars:100 }} + </a> (in {{ sibling.project }}) + </li> + {% endfor %} + </div> + {% endif %} + </ul> + </div> </td> </tr> {% endif %} @@ -277,14 +283,14 @@ function toggle_div(link_id, headers_id, label_show, label_hide) {% else %} <h2>Message</h2> {% endif %} -<div class="comment"> -<div class="meta"> - <span>{{ submission.submitter|personify:project }}</span> - <span class="pull-right">{{ submission.date }} UTC</span> -</div> -<pre class="content"> -{{ submission|commentsyntax }} -</pre> +<div class="patch-message"> + <div class="meta"> + <span>{{ submission.submitter|personify:project }}</span> + <span class="message-date">{{ submission.date }} UTC</span> + </div> + <pre class="content"> + {{ submission|commentsyntax }} + </pre> </div> {% for item in comments %} @@ -293,15 +299,43 @@ function toggle_div(link_id, headers_id, label_show, label_hide) {% endif %} <a name="{{ item.id }}"></a> -<div class="comment"> -<div class="meta"> - <span>{{ item.submitter|personify:project }}</span> - <span class="pull-right">{{ item.date }} UTC | <a href="{% url 'comment-redirect' comment_id=item.id %}" - >#{{ forloop.counter }}</a></span> -</div> -<pre class="content"> -{{ item|commentsyntax }} -</pre> +<div class="patch-message"> + <div class="meta"> + {{ item.submitter|personify:project }} + <span class="message-date">{{ item.date }} UTC | + <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-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"> + <span id="comment-action-icon" class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span> + Unaddressed + </button> + {% endif %} + </div> + {% else %} + <div id="comment-status-bar"> + <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"> + <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 }} + </pre> </div> {% endfor %} diff --git a/patchwork/views/patch.py b/patchwork/views/patch.py index 3e6874a..ac9cb44 100644 --- a/patchwork/views/patch.py +++ b/patchwork/views/patch.py @@ -128,6 +128,7 @@ def patch_detail(request, project_id, msgid): patch.check_set.all().select_related('user'), ) context['submission'] = patch + context['editable'] = editable context['patchform'] = form context['createbundleform'] = createbundleform context['project'] = patch.project
Add new label to show the status of whether a comment is addressed and a button to change the status of the comment. Only users that can edit the patch (submitter, delegate, project maintainers) can change the status of a comment. Clean up submission.html to have hyphen delimited id and class selector names as well. Before [1] and after [2] images for reference. [1] https://imgur.com/NDfcPJy [2] https://imgur.com/kIhohED Signed-off-by: Raxel Gutierrez <raxel@google.com> --- htdocs/css/style.css | 46 +++++- patchwork/templates/patchwork/submission.html | 146 +++++++++++------- patchwork/views/patch.py | 1 + 3 files changed, 134 insertions(+), 59 deletions(-)