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