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 > + {% 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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork