[PATCH v2 1/3] messages: clean up messages and errors containers

Wed Aug 18 21:01:24 AEST 2021

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 <raxel at 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

> +  {% 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.

> +    {% 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