[PATCH v3 0/5] patch-list: improve usability of list action bar
Raxel Gutierrez
raxel at google.com
Wed Aug 4 04:43:52 AEST 2021
ccing list, sorry for the noise again Daniel! :(
--------
Hi Daniel,
> A few bugs:
>
> - if you submit a change with the dropdowns that errors out, and then
> subsequently submit one that succeeds, the error messages are not
> cleared.
Thanks for mentioning this behavior! The behavior for error and update
messages after fetch requests in JS is not polished because of what I
wrote in my v1 cover letter [1 - paragraph 2]:
"For example, the current handling of messages stacks the messages up
as a list of messages. Alternative behaviors like adding timeouts to
requests to remove each update message, simply replacing the existing
messages & errors, or adding to the counter of (e.g. "x+1 patches
updated") can be made."
Given your response, for this case in particular it makes sense to
clear out error messages after any successful action. I will make
those changes in the upcoming v4.
I would appreciate feedback on the general behavior for update/errors
messages though. As mentioned before, should messages stack up,
timeout, or be replaced by each other? Or should they be accumulated
as a counter (e.g. "x" => "x+1 patches updated")? Personally, I think
that any of the first behaviors get the job done. However, the latter
(x+1) and timeout of messages add more complexity which is something
to take into account.
[1] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006953.html
> - You've somehow ended up with two td.patch-delegate:
>
> <td id="patch-delegate:20">
> <td id="patch-delegate:20">
>
>
>
> </td>
> <td id="patch-state:20">
>
> New
>
> </td>
> </tr>
>
> This is throwing off column alignment: delegates now line up under the
> State heading.
Sorry about that! I realized with your message that I somehow lost the
correct changes because I had the correct templating with only one
td.patch-delegate in my `git stash`.
> > For the first patch, the recently released v3.0.0 of the JS cookie
> > library replaces the previous version.
>
> - the js.cookie.min.js file is still corrupted and I redownloaded it
> from a CDN. I'm not sure where the bug lies here and it's probably
> not something you need to fix, but a link to the source in the commit
> message would be nice.
I replied on list [2] for the link on v2, but I will also add the link
to download the file to the commit message.
[2] https://lists.ozlabs.org/pipermail/patchwork/2021-July/006992.html
> > For the fifth patch, the inline dropdowns are changed so that they are
> > only visible to logged in users. Also, upon any unsuccessful update
> > requests, the dropdown selection reverts to its previous selection.
> > Since update requests to a patch’s state and delegate fields fail for
> > unauthorized users, this behavior covers the case where unauthorized
> > users don't see the current state of the db after they change the
> > dropdown selection. This behavior is a useful example of patch four’s
> > change of returning whether an update request is successful.
>
> - they're visible to logged in users but still suggest that if I am a
> regular user that I have the ability to change anyone's patch
> state/delegate. I am still a bit dubious overall about inline
> dropdowns in general, although I am cognisant that it's much more
> discoverable this way. Either way, IMO dropdowns need to be
> restricted to editable patches.
I did make the implementation where only users with permissions can
see the dropdown, however the loading of the page is significantly
slower. So, I feel that the safety measures added for logged in users
would be best. The message could be more explicit and say the exact
permissions required, which would prevent less mistakes in the future
as users are more informed. Nonetheless, I'm interested in knowing how
you feel about this considering the slower page loads. My earlier
reply (that I forgot to cc) in the v2 patch [3].
[3] https://lists.ozlabs.org/pipermail/patchwork/2021-August/007020.html
More information about the Patchwork
mailing list