| Message ID | 20210817213142.2962558-2-raxel@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rest: add rest.js to handle PATCH requests & respective responses | expand |
| Related | show |
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@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
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 <raxel@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. 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 <stephen@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
Stephen Finucane <stephen@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 <raxel@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 <stephen@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
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); +} + 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 + --> + {% spaceless %} + <ul class="messages"> + {% if messages %} + {% for message in messages %} + <li{% if message.tags %} class="{{ message.tags }}"{% endif %}>{{ message }}</li> + {% endfor %} + {% endif %} + </ul> + <div id="errors"> + {% if errors %} + <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 %}
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@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(-)