Sorry forgot to cc the list for the reply below; apologies for the noise for those who already received this ------------------- Hi,
Thanks for the review! :) > This is pretty cool. My initial thoughts are as follows: > > Can we make these only appear editable if you are either: > - logged in, > - or ideally, if you have the rights to change them > > Currently I see a bunch of drop downs when I access the page as a > non-logged-in user where I couldn't possibly change the state or > delegate. I tested the ideal case where users can only see the dropdown if they have rights to change it. The loading of the page is significantly slower as the permissions for each patch need to be checked. I also tested the logged in state variant and it doesn't affect the loading of the page significantly. Nonetheless, I think that the error messages that are rendered at the top of the page are good enough for those specific scenarios. > I'm umming and ahing about whether I want to gate this feature entirely > behind a user-specific setting: check out commit 6e32965b04cc ("'mpe > mode': click to copy patch IDs") for an example. Or whether it should be > on by default but disable-able via a setting. I'm a bit worried that it > makes rows wider and our power users have tended to get a bit irritated > when we do things that break their muscle memory. I can't speak for power users but I feel the change in the horizontal space is not very significant. > Also, if there's an error setting a value (e.g. I'm not logged in) it > would be nice if the value reset so that I only see what the actual > state in the db. Since not logged in users now can't see the dropdowns, this case is covered. Nonetheless, I changed the functionality so that when requests fail, the dropdown selection reverts to its previous selection. The requests fails for unauthorized users making changes to the patches, so it will cover the case of logged in users without permissions as well. Best, Raxel _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork