Description
Josef Moravec
2016-10-04 12:56:59 UTC
Created attachment 56031 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected Created attachment 56047 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected Created attachment 56050 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected I get this error on the circulation page after applying this patch: DBIx::Class::ResultSource::_resolve_join(): No such relationship manager on Message at kohaclone/Koha/Objects.pm line 168 Have I missed a step in the test plan? You will need to apply 17397 first and also update db structure and update dbix class files. Sorry to not mention it here... I'll add a schema change to 17397... Created attachment 57216 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected Bug 17397 introduced a stray closing bracket to the markup on line 859 of circulation.tt: + [% END %]> I wonder if this patch should include the fix? Created attachment 57391 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected (In reply to Owen Leonard from comment #8) > Bug 17397 introduced a stray closing bracket to the markup on line 859 of > circulation.tt: > > + [% END %]> > > I wonder if this patch should include the fix? Good catch, thanks. I've fixed it and updated the patch. I like that this patch simplifies the modal view. I disagree that the "Delete" link should be converted to a button. It doesn't occur very often in the interface, but I think the more common pattern is to use links for actions which are "inline," or part of a line of text. Thanks for testing! I just followed the changes which were made on pay links in circulation by bug 16810, but as I see, it was a bit by accident... I have no strong opinion to make delete link a button... But I don't like the link to be in square brackets... (In reply to Josef Moravec from comment #12) > I have no strong opinion to make delete link a button... But I don't like > the link to be in square brackets... I agree. The brackets were more necessary before the link had an icon associated with it. We should look into adapting the btn-link class (after Bug 16239 probably) to work in instances like this so that the icon and link text don't wrap separately. Created attachment 57408 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected I did just remove btn class from delete link and make it a bit larger to size btn-small... this approach is taken from advanced search toolbar, as it will be changed to btn-link by 16239 (as it does with the links in advanced search toolbar) Created attachment 57409 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected Created attachment 58240 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 58306 [details] [review] Bug 17398: Enhance circulation messages UI Test plan: 1) Apply patch 2) Add same circulation messages, note that both buttons are in bootstrap style and the whole form is a bit cleaner 3) Confirm that adding works as expected 4) Try to delete some of your messages, note the delete link is also button now 5) Confirm that deleting works as expected Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 17.05, thanks Josef! This won't get ported back to 16.11.x as it is an enhancement. |