Base modal template #1440
Base modal template #1440
Conversation
mad-anne
requested review from
patrys,
salwator,
maarcingebala,
dominik-zeglen,
akjanik and
koradon
saleor/dashboard/discount/views.py
Outdated
| @@ -124,7 +124,7 @@ def voucher_delete(request, pk): | |||
| instance.delete() | |||
| messages.success( | |||
| request, | |||
| pgettext_lazy('Voucher message', 'Removed voucher %s') % (instance,)) | |||
| pgettext_lazy('Voucher message', 'Removed voucher %s') % instance) | |||
There was a problem hiding this comment.
Please always explicitly pass either a tuple or a dict when interpolating strings with %.
There was a problem hiding this comment.
Ok, I'll restore it.
Codecov Report
@@ Coverage Diff @@
## master #1440 +/- ##
=========================================
Coverage ? 83.22%
=========================================
Files ? 128
Lines ? 5848
Branches ? 674
=========================================
Hits ? 4867
Misses ? 820
Partials ? 161
Continue to review full report at Codecov.
|
templates/dashboard/base_modal.html
Outdated
| {% block cancel_action %} {% trans "Cancel" context "Dashboard modal cancel action" %} {% endblock %} | ||
| </a> | ||
| <button type="submit" class="modal-action btn-flat"> | ||
| {% block primary_action %} {% trans "Submit" context "Dashboard modal primary action" %} {% endblock %} |
There was a problem hiding this comment.
I'm not a fan of this. Do we ever want to display a generic "Submit" button (or "Wyślij" when localized)?
There was a problem hiding this comment.
I suppose we don't. I could omit this text and leave an empty block to force passing text value for primary action.
| {% endblock %} | ||
|
|
||
| {% block content %} | ||
| {% trans "Are you sure?" context "Modal promote to staff member text" %} |
There was a problem hiding this comment.
I realize this string was already here but I feel that "Are you sure?" should really restate the thing that's about to happen. In its current state it will also look awkward when localized ("Czy na pewno?" to avoid gendered forms).
There was a problem hiding this comment.
You are right. We can display instead Are you sure you want to promote {{ customer }} to staff member? as we keep doing in confirm_delete modal templates. Referring to those templates, I would rename all of them from confirm_delete to confirm_remove or even remove to keep consistent naming of removal actions.
templates/dashboard/base_modal.html
Outdated
| {% load i18n %} | ||
|
|
||
| <form | ||
| role="form" method="post" action="{% block action_url %}#!{% endblock %}" |
There was a problem hiding this comment.
We use 2-spaces indentation in our templates.
templates/dashboard/base_modal.html
Outdated
|
|
||
| <form | ||
| role="form" method="post" action="{% block action_url %}#!{% endblock %}" | ||
| class="{% block form_class %}form-async{% endblock %}" novalidate> |
There was a problem hiding this comment.
Here we define class form-async, but in some modal templates in this PR we override it with an empty block and in some other, we don't. Why is that? Is that inconsistency in how our modals work?
There was a problem hiding this comment.
The reason is that when we perform remove actions we shouldn't make ajax request and go back to model details page (like form-async class treats such modals), but rather simply process form and redirect to address returned from server (usually a list).
There was a problem hiding this comment.
Makes sense but it could be documented because either we forget about it or someone else creating a new template will not know how to use it. A comment in this template would be useful.
Add base modal template for all modals used in dashboard.
Closes #1352
Pull Request Checklist
pycodestyle,pydocstyle,pylint.eslint.