| Message ID | 20210719233428.2045872-4-raxel@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | patch-list: improve usability of list action bar | expand |
Raxel Gutierrez wrote: > [Subject: patch-list: improved list action bar ui] nit: like in the previous patches, this should be in the imperative mood. (The same applies to the remaining patches but I'll stop mentioning it at this point in the series.) The commit subject is a good place to put a summary of the benefit this patch brings. All patches believe they are improving something :), but what particular improvement are we implementing here? The cover letter explains it as the third patch that changes the design to a more action bar UI so perhaps we can use a subject line in that direction? E.g. patch-list: style modification forms as an action bar > Added styling to the new patch list html code to make the change > property and bundle action forms more usable. Post in mailing list about > the design mockups shows how the new UI looks[1]. > > [1] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006943.html Same nit about being self-contained. On the other hand, a before and after image would be helpful --- maybe it makes sense to use a 'Link:' line pointing e.g. to an imgur URL or slides link showing the before-and-after? Or one can fake it with ascii art. ;-) > Signed-off-by: Raxel Gutierrez <raxel@google.com> > --- > htdocs/css/style.css | 64 +++++++++++++++++++++++++++----------------- > patchwork/forms.py | 27 ++++++++++++++----- > 2 files changed, 59 insertions(+), 32 deletions(-) > > diff --git a/htdocs/css/style.css b/htdocs/css/style.css > index 243caa0..8746083 100644 > --- a/htdocs/css/style.css > +++ b/htdocs/css/style.css [...] > @@ -346,40 +346,54 @@ table.bundlelist td > padding-right: 2em; > } > > +.patch-list-actions { > + width: 100%; > + display: inline-flex; > + flex-wrap: wrap; > + justify-content: space-between; > +} Yay for flex boxes! > --- a/patchwork/forms.py > +++ b/patchwork/forms.py > @@ -122,10 +122,11 @@ class PatchForm(forms.ModelForm): > > > class OptionalModelChoiceField(forms.ModelChoiceField): > - no_change_choice = ('*', 'no change') > + no_change_choice = ('*', 'No change') > to_field_name = None > > - def __init__(self, *args, **kwargs): > + def __init__(self, placeholder, *args, **kwargs): > + self.no_change_choice = ('*', placeholder) This is potentially dangerous because with an unadapted caller, a parameter intended as the first element of *args can end up being passed as placeholder. One way to avoid that would be to use a PEP-3102 keyword-only argument. Support for those were introduced in Python 3.0, so they should be safe to use: def __init__(self, *args, placeholder, **kwargs): There are only two callers and you correctly adapt them below; still, it seems like a good habit to get into. > super(OptionalModelChoiceField, self).__init__( > initial=self.no_change_choice[0], *args, **kwargs) > > @@ -161,17 +162,29 @@ class OptionalBooleanField(forms.TypedChoiceField): > class MultiplePatchForm(forms.Form): > action = 'update' > archived = OptionalBooleanField( > - choices=[('*', 'no change'), ('True', 'Archived'), > - ('False', 'Unarchived')], > + choices=[('*', 'No change'), ('True', 'Archive'), > + ('False', 'Unarchive')], > coerce=lambda x: x == 'True', > - empty_value='*') > + empty_value='*', > + label="Archived") > > def __init__(self, project, *args, **kwargs): > super(MultiplePatchForm, self).__init__(*args, **kwargs) > self.fields['delegate'] = OptionalModelChoiceField( > - queryset=_get_delegate_qs(project=project), required=False) > + queryset=_get_delegate_qs(project=project), > + placeholder="Delegate to", > + label="Delegate to", > + required=False) > self.fields['state'] = OptionalModelChoiceField( > - queryset=State.objects.all()) > + queryset=State.objects.all(), > + placeholder="Change state", > + label="Change state") > + self.fields['state'].widget.attrs.update( > + {'class': 'change-property'}) > + self.fields['delegate'].widget.attrs.update( > + {'class': 'change-property'}) > + self.fields['archived'].widget.attrs.update( > + {'class': 'archive-patch'}) Should OptionalModelChoiceField be responsible for setting 'class' instead of the caller having to do it? (Genuine question; I haven't thought it through myself.) Thanks, Jonathan
Hi, > Should OptionalModelChoiceField be responsible for setting 'class' > instead of the caller having to do it? (Genuine question; I haven't > thought it through myself.) In my in-progress v2, I moved the functionality to the __init__ calls of each respective field object so that it's more flexible within the MultiplePatchForm calls.
diff --git a/htdocs/css/style.css b/htdocs/css/style.css index 243caa0..8746083 100644 --- a/htdocs/css/style.css +++ b/htdocs/css/style.css @@ -1,3 +1,7 @@ +:root { + --light-color: #F7F7F7; +} + h2 { font-size: 25px; margin: 18px 0 18px 0; @@ -122,10 +126,6 @@ a.colinactive:hover { div.filters { } -div.patchforms { - margin-top: 1em; -} - /* list order manipulation */ table.patchlist tr.draghover { @@ -149,7 +149,7 @@ input#reorder-change { .paginator { text-align: right; clear: both; - margin: 8px 0 15px; + margin: 8px 0 15px; } .paginator .prev-na, @@ -346,40 +346,54 @@ table.bundlelist td padding-right: 2em; } +.patch-list-actions { + width: 100%; + display: inline-flex; + flex-wrap: wrap; + justify-content: space-between; +} + /* forms that appear for a patch */ +.patchforms { + display: inline-flex; + flex-wrap: wrap; + margin: 16px 0px; +} + div.patchform { - border: thin solid #080808; - padding-left: 0.6em; - padding-right: 0.6em; - float: left; - margin: 0.5em 5em 0.5em 10px; + display: flex; + flex-wrap: wrap; + align-items: center; } -div.patchform h3 { - margin-top: 0em; - margin-left: -0.6em; - margin-right: -0.6em; - padding: 0.3em 0.3em 0.3em 0.6em; - background-color: #222; - color: #999; - font-size: 100%; +.change-property, .archive-patch, .add-bundle { + padding: 4px; + margin-right: 8px; + box-sizing: border-box; + border-radius: 4px; + background-color: var(--light-color); } -div.patchform ul { - list-style-type: none; - padding-left: 0.2em; - margin-top: 0em; +.patchform-submit { + font-weight: bold; + padding: 4px; +} + +#patchform-bundle, #add-to-bundle, #remove-bundle { + margin-left: 16px; } -/* forms */ -table.form { +.create-bundle { + padding: 4px; + margin-right: 8px; + box-sizing: border-box; + border-radius: 4px; } span.help_text { font-size: 80%; } - table.form td { padding: 0.6em; vertical-align: top; diff --git a/patchwork/forms.py b/patchwork/forms.py index 24322c7..b684026 100644 --- a/patchwork/forms.py +++ b/patchwork/forms.py @@ -122,10 +122,11 @@ class PatchForm(forms.ModelForm): class OptionalModelChoiceField(forms.ModelChoiceField): - no_change_choice = ('*', 'no change') + no_change_choice = ('*', 'No change') to_field_name = None - def __init__(self, *args, **kwargs): + def __init__(self, placeholder, *args, **kwargs): + self.no_change_choice = ('*', placeholder) super(OptionalModelChoiceField, self).__init__( initial=self.no_change_choice[0], *args, **kwargs) @@ -161,17 +162,29 @@ class OptionalBooleanField(forms.TypedChoiceField): class MultiplePatchForm(forms.Form): action = 'update' archived = OptionalBooleanField( - choices=[('*', 'no change'), ('True', 'Archived'), - ('False', 'Unarchived')], + choices=[('*', 'No change'), ('True', 'Archive'), + ('False', 'Unarchive')], coerce=lambda x: x == 'True', - empty_value='*') + empty_value='*', + label="Archived") def __init__(self, project, *args, **kwargs): super(MultiplePatchForm, self).__init__(*args, **kwargs) self.fields['delegate'] = OptionalModelChoiceField( - queryset=_get_delegate_qs(project=project), required=False) + queryset=_get_delegate_qs(project=project), + placeholder="Delegate to", + label="Delegate to", + required=False) self.fields['state'] = OptionalModelChoiceField( - queryset=State.objects.all()) + queryset=State.objects.all(), + placeholder="Change state", + label="Change state") + self.fields['state'].widget.attrs.update( + {'class': 'change-property'}) + self.fields['delegate'].widget.attrs.update( + {'class': 'change-property'}) + self.fields['archived'].widget.attrs.update( + {'class': 'archive-patch'}) def save(self, instance, commit=True): opts = instance.__class__._meta
Added styling to the new patch list html code to make the change property and bundle action forms more usable. Post in mailing list about the design mockups shows how the new UI looks[1]. [1] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006943.html Signed-off-by: Raxel Gutierrez <raxel@google.com> --- htdocs/css/style.css | 64 +++++++++++++++++++++++++++----------------- patchwork/forms.py | 27 ++++++++++++++----- 2 files changed, 59 insertions(+), 32 deletions(-)