Stephen Finucane <step...@that.guru> writes:
> On Wed, 2021-08-18 at 12:01 +0100, Stephen Finucane wrote:
>> On Tue, 2021-08-17 at 21:31 +0000, Raxel Gutierrez wrote:
>> > Refactor the messages container to make use of message.tags [1] which
>> > allows for more customization for each level (e.g. success, warning,
>> > error, etc.) of a message through CSS selectors. As basic proof of
>> > concept, customize the text color of each existing message level. Also,
>> > this addition resolves a TODO by stephenfin in the previous code.
>> >
>> > Move the errors container after the messages container in the DOM in the
>> > base.html template so that every template can share the same errors
>> > container. Also, add a background color to the errors container so that
>> > both containers blend in as a uniform block. Add bullet points to each
>> > error item in the list of errors.
>> >
>> > Change both the messages and errors containers to always exist in
>> > the DOM. With this, the addition of update and error messages is simpler
>> > because it eliminates the need to create the containers if they don't
>> > exist. These changes will be useful in a following patch that introduces
>> > an internal JS module to make client-side requests to the REST API.
>> >
>> > [1]
>> > https://docs.djangoproject.com/en/3.2/ref/contrib/messages/#message-tags
>> >
>> > Signed-off-by: Raxel Gutierrez <ra...@google.com>
>> > ---
>> > htdocs/css/style.css | 43 ++++++++++++++-----
>> > patchwork/templates/patchwork/list.html | 10 -----
>> > .../patchwork/user-link-confirm.html | 5 +--
>> > patchwork/templates/patchwork/user-link.html | 4 +-
>> > templates/base.html | 30 +++++++++----
>> > 5 files changed, 58 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/htdocs/css/style.css b/htdocs/css/style.css
>> > index 46a91ee8..f30988e0 100644
>> > --- a/htdocs/css/style.css
>> > +++ b/htdocs/css/style.css
>> > @@ -1,3 +1,9 @@
>> > +:root {
>> > + --success-color:rgb(92, 184, 92);
>> > + --warning-color:rgb(240, 173, 78);
>> > + --danger-color:rgb(217, 83, 79);
>> > +}
>>
>> Neat. I didn't know CSS variables were a thing now, but it seems they've been
>> available for a few years. TIL.
>>
>> > +
>> > h2 {
>> > font-size: 25px;
>> > margin: 18px 0 18px 0;
>> > @@ -78,14 +84,27 @@ dl dt {
>> > }
>> >
>> > /* messages */
>> > -#messages {
>> > +.messages {
>> > background: #e0e0f0;
>> > - margin: 0.5em 1em 0.0em 0.5em;
>> > + margin: 0.5em 1em 0.0em 1em;
>> > padding: 0.3em;
>> > + list-style-type: none;
>> > +}
>> > +
>> > +.messages:empty {
>> > + display: none;
>> > +}
>> > +
>> > +.messages .success {
>> > + color: var(--success-color);
>> > +}
>> > +
>> > +.messages .warning {
>> > + color: var(--warning-color);
>> > }
>> >
>> > -#messages .message {
>> > - color: green;
>> > +.messages .error {
>> > + color: var(--danger-color);
>> > }
>> >
>> > .filters {
>> > @@ -421,13 +440,17 @@ table.loginform {
>> > }
>> >
>> > /* form errors */
>> > -.errorlist {
>> > - color: red;
>> > - list-style-type: none;
>> > - padding-left: 0.2em;
>> > - margin: 0em;
>> > +#errors {
>> > + background: #e0e0f0;
>> > + margin: 0em 1em 0.5em 1em;
>> > + padding: 0.3em;
>> > }
>> > -.error {
>> > +
>> > +#errors:empty {
>> > + display: none;
>> > +}
>> > +
>> > +.error-list, .errorlist {
>> > color: red;
>> > }
>> >
>> > diff --git a/patchwork/templates/patchwork/list.html
>> > b/patchwork/templates/patchwork/list.html
>> > index 5d3d82aa..6efbed26 100644
>> > --- a/patchwork/templates/patchwork/list.html
>> > +++ b/patchwork/templates/patchwork/list.html
>> > @@ -8,16 +8,6 @@
>> >
>> > {% block body %}
>> >
>> > -{% if errors %}
>> > -<p>The following error{{ errors|length|pluralize:" was,s were" }}
>> > encountered
>> > -while updating patches:</p>
>> > -<ul class="errorlist">
>> > -{% for error in errors %}
>> > - <li>{{ error }}</li>
>> > -{% endfor %}
>> > -</ul>
>> > -{% endif %}
>> > -
>> > {% include "patchwork/partials/patch-list.html" %}
>> >
>> > {% endblock %}
>> > diff --git a/patchwork/templates/patchwork/user-link-confirm.html
>> > b/patchwork/templates/patchwork/user-link-confirm.html
>> > index 79678f64..a6d671f3 100644
>> > --- a/patchwork/templates/patchwork/user-link-confirm.html
>> > +++ b/patchwork/templates/patchwork/user-link-confirm.html
>> > @@ -5,12 +5,9 @@
>> >
>> > {% block body %}
>> >
>> > -{% if errors %}
>> > -<p>{{ errors }}</p>
>> > -{% else %}
>> > +{% if not errors %}
>> > <p>You have successfully linked the email address {{ person.email }} to
>> > your Patchwork account</p>
>> > -
>> > {% endif %}
>> > <p>Back to <a href="{% url 'user-profile' %}">your
>> > profile</a>.</p>
>> > diff --git a/patchwork/templates/patchwork/user-link.html
>> > b/patchwork/templates/patchwork/user-link.html
>> > index bf331520..c0595bdc 100644
>> > --- a/patchwork/templates/patchwork/user-link.html
>> > +++ b/patchwork/templates/patchwork/user-link.html
>> > @@ -9,12 +9,12 @@
>> > on the link provided in the email to confirm that this address belongs to
>> > you.</p>
>> > {% else %}
>> > + <p>There was an error submitting your link request:</p>
>> > {% if form.errors %}
>> > - <p>There was an error submitting your link request.</p>
>> > {{ form.non_field_errors }}
>> > {% endif %}
>> > {% if error %}
>> > - <ul class="errorlist"><li>{{error}}</li></ul>
>> > + <ul class="error-list"><li>{{error}}</li></ul>
>> > {% endif %}
>> >
>> > <form action="{% url 'user-link' %}" method="post">
>> > diff --git a/templates/base.html b/templates/base.html
>> > index a95a11b0..3a0825ac 100644
>> > --- a/templates/base.html
>> > +++ b/templates/base.html
>> > @@ -104,15 +104,29 @@
>> > </div>
>> > </div>
>> > </nav>
>> > -{% if messages %}
>> > - <div id="messages">
>> > - {% for message in messages %}
>> > - {# TODO(stephenfin): Make use of message.tags when completely #}
>> > - {# converted to django.contrib.messages #}
>> > - <div class="message">{{ message }}</div>
>> > - {% endfor %}
>> > + <!--
>> > + spaceless tag is used to remove automatically added whitespace so
>> > that the container
>> > + is truly considered empty by the `:empty` pseudo-class that is used
>> > for styling
>> > + -->
>>
>> nit: 'spaceless' is a Django tag and as such does not appear in the rendered
>> HTML. However, this HTML comment does. I think you want to use a Django
>> comment
>> [1]:
>>
>> {#
>> spaceless tag is used ...
>> #}
>>
>> [1] https://docs.djangoproject.com/en/3.2/ref/templates/language/#comments
I found I needed to use '{% comment %}'/'{% endcomment %}' - I think '{#
#}' might be from Jinja2 which hasn't fully displaced the old django
template language yet...
Anyway I did that and applied the change. Thanks Raxel and Stephen.
Kind regards,
Daniel
>>
>> > + {% spaceless %}
>> > + <ul class="messages">
>> > + {% if messages %}
>>
>> Is there any particular reason you've inverted the logic here, rather than
>> leaving the entire 'messages' block inside the conditional as before:
>>
>> {% if messages %}
>> <ul class="messages">
>> ...
>> </ul>
>> {% endif %}
>>
>> This is what necessitates the need for the '.messages:empty' CSS code and
>> well
>> as the use of the 'spaceless' tag.
>
> Apologies, I see what you're trying to do in patch 3 and also note that you
> specifically called this out in the commit message /o\ You can ignore this
> comment as well as its sibling below.
>
> I'd still like to change the HTML comment to a Django template comment, but we
> can do that at merge time to avoid another respin. Other than that, LGTM:
>
> Reviewed-by: Stephen Finucane <step...@that.guru>
>
> I'll give Daniel a chance to come back around to this and merge it since he'd
> reviewed it previously.
>
> Stephen
>
>> > + {% for message in messages %}
>> > + <li{% if message.tags %} class="{{ message.tags }}"{% endif %}>{{
>> > message }}</li>
>> > + {% endfor %}
>> > + {% endif %}
>> > + </ul>
>> > + <div id="errors">
>> > + {% if errors %}
>>
>> As above, any reason to have things this way rather than making the entire
>> block
>> conditional on whether 'errors' is present in the context?
>>
>> {% if errors %}
>> <div id="errors">
>> ...
>> </div>
>> {% endif %}
>>
>> > + <p>The following error{{ errors|length|pluralize:" was,s were" }}
>> > encountered:</p>
>> > + <ul class="error-list">
>> > + {% for error in errors %}
>> > + <li>{{ error }}</li>
>> > + {% endfor %}
>> > + </ul>
>> > + {% endif %}
>> > </div>
>> > -{% endif %}
>> > + {% endspaceless %}
>> > <div id="main-content" class="container-fluid">
>> > {% block body %}
>> > {% endblock %}
>>
>> Other than the comments above, this looks pretty good to me. I'm happy to
>> address these comments when merging if you agree.
>>
>> Cheers,
>> Stephen
>>
>> _______________________________________________
>> 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
