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