[PATCH 3/5] patch-list: improved list action bar ui

[Jonathan Nieder]

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 at 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