diff mbox series

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

Message ID 20210817003225.2813222-2-raxel@google.com
State Superseded
Headers show
Series rest: add rest.js to handle PATCH requests & respective responses | expand
Related show

Commit Message

Raxel Gutierrez Aug. 17, 2021, 12:32 a.m. UTC
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.

Modularize the errors container with a new partial template errors.html
that makes it easier to reuse errors in various pages. These changes
make the code more readable and ready for change.

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@google.com>
---
 htdocs/css/style.css                          | 26 ++++++++++++++++---
 patchwork/templates/patchwork/list.html       | 10 +------
 .../templates/patchwork/partials/errors.html  | 10 +++++++
 patchwork/templates/patchwork/submission.html |  2 ++
 patchwork/templates/patchwork/user-link.html  |  2 +-
 templates/base.html                           | 22 +++++++++-------
 6 files changed, 49 insertions(+), 23 deletions(-)
 create mode 100644 patchwork/templates/patchwork/partials/errors.html

Comments

Daniel Axtens Aug. 17, 2021, 3:06 a.m. UTC | #1
Raxel Gutierrez <raxel@google.com> writes:

> 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.
>
> Modularize the errors container with a new partial template errors.html
> that makes it easier to reuse errors in various pages. These changes
> make the code more readable and ready for change.
>
> 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.

(With the rest of your other addressed/unaddressed series applied,) I'm
seeing the list bullets on errors and messages, and error display is
still a little odd:

https://imgur.com/a/y8vBckQ
https://imgur.com/a/jN8b3KA

Could you please hide the list bullets? (and maybe make the errors all
within a box? I don't know if the current display is by design or not.)

Other than that, looking promising! :)

Kind regards,
Daniel

>
> [1] https://docs.djangoproject.com/en/3.2/ref/contrib/messages/#message-tags
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  htdocs/css/style.css                          | 26 ++++++++++++++++---
>  patchwork/templates/patchwork/list.html       | 10 +------
>  .../templates/patchwork/partials/errors.html  | 10 +++++++
>  patchwork/templates/patchwork/submission.html |  2 ++
>  patchwork/templates/patchwork/user-link.html  |  2 +-
>  templates/base.html                           | 22 +++++++++-------
>  6 files changed, 49 insertions(+), 23 deletions(-)
>  create mode 100644 patchwork/templates/patchwork/partials/errors.html
>
> diff --git a/htdocs/css/style.css b/htdocs/css/style.css
> index 46a91ee8..bcc57b2c 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);
> +}
> +
>  h2 {
>      font-size: 25px;
>      margin: 18px 0 18px 0;
> @@ -78,14 +84,26 @@ dl dt {
>  }
>  
>  /* messages */
> -#messages {
> +.messages {
>      background: #e0e0f0;
>      margin: 0.5em 1em 0.0em 0.5em;
>      padding: 0.3em;
>  }
>  
> -#messages .message {
> -    color: green;
> +.messages:empty {
> +    display: none;
> +}
> +
> +.messages .success {
> +    color: var(--success-color);
> +}
> +
> +.messages .warning {
> +    color: var(--warning-color);
> +}
> +
> +.messages .error {
> +    color: var(--danger-color);
>  }
>  
>  .filters {
> @@ -421,7 +439,7 @@ table.loginform {
>  }
>  
>  /* form errors */
> -.errorlist {
> +.error-list {
>      color: red;
>      list-style-type: none;
>      padding-left: 0.2em;
> diff --git a/patchwork/templates/patchwork/list.html b/patchwork/templates/patchwork/list.html
> index 5d3d82aa..91387cf0 100644
> --- a/patchwork/templates/patchwork/list.html
> +++ b/patchwork/templates/patchwork/list.html
> @@ -8,15 +8,7 @@
>  
>  {% 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/errors.html" %}
>  
>  {% include "patchwork/partials/patch-list.html" %}
>  
> diff --git a/patchwork/templates/patchwork/partials/errors.html b/patchwork/templates/patchwork/partials/errors.html
> new file mode 100644
> index 00000000..86eec121
> --- /dev/null
> +++ b/patchwork/templates/patchwork/partials/errors.html
> @@ -0,0 +1,10 @@
> +<div id="errors">
> +    {% if errors %}
> +        <p>The following error{{ errors|length|pluralize:" was,s were" }} encountered while updating patches:</p>
> +        <ul class="error-list">
> +        {% for error in errors %}
> +            <li>{{ error }}</li>
> +        {% endfor %}
> +        </ul>
> +    {% endif %}
> +</div>
> diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
> index 66efa0b5..3b65f81d 100644
> --- a/patchwork/templates/patchwork/submission.html
> +++ b/patchwork/templates/patchwork/submission.html
> @@ -14,6 +14,8 @@
>  
>  {% block body %}
>  
> +{% include "patchwork/partials/errors.html" %}
> +
>  <div>
>    {% include "patchwork/partials/download-buttons.html" %}
>    <h1>{{ submission.name }}</h1>
> diff --git a/patchwork/templates/patchwork/user-link.html b/patchwork/templates/patchwork/user-link.html
> index bf331520..e23e69d5 100644
> --- a/patchwork/templates/patchwork/user-link.html
> +++ b/patchwork/templates/patchwork/user-link.html
> @@ -14,7 +14,7 @@ you.</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..a9d0906b 100644
> --- a/templates/base.html
> +++ b/templates/base.html
> @@ -104,15 +104,19 @@
>      </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 %}
> -  </div>
> -{% endif %}
> +  <!--
> +    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
> +  -->
> +  {% spaceless %}
> +  <ul class="messages">
> +  {% if messages %}
> +    {% for message in messages %}
> +    <li{% if message.tags %} class="{{ message.tags }}"{% endif %}>{{ message }}</li>
> +    {% endfor %}
> +  {% endif %}
> +  </ul>
> +  {% endspaceless %}
>    <div id="main-content" class="container-fluid">
>  {% block body %}
>  {% endblock %}
> -- 
> 2.33.0.rc1.237.g0d66db33f3-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
diff mbox series

Patch

diff --git a/htdocs/css/style.css b/htdocs/css/style.css
index 46a91ee8..bcc57b2c 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);
+}
+
 h2 {
     font-size: 25px;
     margin: 18px 0 18px 0;
@@ -78,14 +84,26 @@  dl dt {
 }
 
 /* messages */
-#messages {
+.messages {
     background: #e0e0f0;
     margin: 0.5em 1em 0.0em 0.5em;
     padding: 0.3em;
 }
 
-#messages .message {
-    color: green;
+.messages:empty {
+    display: none;
+}
+
+.messages .success {
+    color: var(--success-color);
+}
+
+.messages .warning {
+    color: var(--warning-color);
+}
+
+.messages .error {
+    color: var(--danger-color);
 }
 
 .filters {
@@ -421,7 +439,7 @@  table.loginform {
 }
 
 /* form errors */
-.errorlist {
+.error-list {
     color: red;
     list-style-type: none;
     padding-left: 0.2em;
diff --git a/patchwork/templates/patchwork/list.html b/patchwork/templates/patchwork/list.html
index 5d3d82aa..91387cf0 100644
--- a/patchwork/templates/patchwork/list.html
+++ b/patchwork/templates/patchwork/list.html
@@ -8,15 +8,7 @@ 
 
 {% 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/errors.html" %}
 
 {% include "patchwork/partials/patch-list.html" %}
 
diff --git a/patchwork/templates/patchwork/partials/errors.html b/patchwork/templates/patchwork/partials/errors.html
new file mode 100644
index 00000000..86eec121
--- /dev/null
+++ b/patchwork/templates/patchwork/partials/errors.html
@@ -0,0 +1,10 @@ 
+<div id="errors">
+    {% if errors %}
+        <p>The following error{{ errors|length|pluralize:" was,s were" }} encountered while updating patches:</p>
+        <ul class="error-list">
+        {% for error in errors %}
+            <li>{{ error }}</li>
+        {% endfor %}
+        </ul>
+    {% endif %}
+</div>
diff --git a/patchwork/templates/patchwork/submission.html b/patchwork/templates/patchwork/submission.html
index 66efa0b5..3b65f81d 100644
--- a/patchwork/templates/patchwork/submission.html
+++ b/patchwork/templates/patchwork/submission.html
@@ -14,6 +14,8 @@ 
 
 {% block body %}
 
+{% include "patchwork/partials/errors.html" %}
+
 <div>
   {% include "patchwork/partials/download-buttons.html" %}
   <h1>{{ submission.name }}</h1>
diff --git a/patchwork/templates/patchwork/user-link.html b/patchwork/templates/patchwork/user-link.html
index bf331520..e23e69d5 100644
--- a/patchwork/templates/patchwork/user-link.html
+++ b/patchwork/templates/patchwork/user-link.html
@@ -14,7 +14,7 @@  you.</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..a9d0906b 100644
--- a/templates/base.html
+++ b/templates/base.html
@@ -104,15 +104,19 @@ 
     </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 %}
-  </div>
-{% endif %}
+  <!--
+    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
+  -->
+  {% spaceless %}
+  <ul class="messages">
+  {% if messages %}
+    {% for message in messages %}
+    <li{% if message.tags %} class="{{ message.tags }}"{% endif %}>{{ message }}</li>
+    {% endfor %}
+  {% endif %}
+  </ul>
+  {% endspaceless %}
   <div id="main-content" class="container-fluid">
 {% block body %}
 {% endblock %}