Bug 17398 - Enhance circulation message UI
Summary: Enhance circulation message UI
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Josef Moravec
QA Contact: Testopia
URL:
Keywords:
Depends on: 17397
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-04 12:56 UTC by Josef Moravec
Modified: 2017-12-07 22:17 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17398: Enhance circulation messages UI (6.16 KB, patch)
2016-10-04 13:02 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (6.16 KB, patch)
2016-10-05 05:38 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (6.46 KB, patch)
2016-10-05 09:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (6.49 KB, patch)
2016-11-04 15:50 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (6.93 KB, patch)
2016-11-09 15:13 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (6.93 KB, patch)
2016-11-10 06:22 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (6.92 KB, patch)
2016-11-10 06:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (7.05 KB, patch)
2016-12-16 09:37 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17398: Enhance circulation messages UI (7.13 KB, patch)
2016-12-20 16:23 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2016-10-04 12:56:59 UTC

    
Comment 1 Josef Moravec 2016-10-04 13:02:42 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
Comment 2 Josef Moravec 2016-10-05 05:38:46 UTC
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
Comment 3 Josef Moravec 2016-10-05 09:18:50 UTC
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
Comment 4 Owen Leonard 2016-10-05 14:03:37 UTC
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?
Comment 5 Josef Moravec 2016-10-05 14:27:43 UTC
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...
Comment 6 Josef Moravec 2016-11-04 15:50:00 UTC
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
Comment 7 Josef Moravec 2016-11-04 15:51:18 UTC
Bug 17397 is in master now

I've rebased on master (especially because Bug 17365)
Comment 8 Owen Leonard 2016-11-09 14:47:39 UTC
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?
Comment 9 Josef Moravec 2016-11-09 15:13:33 UTC
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
Comment 10 Josef Moravec 2016-11-09 15:14:36 UTC
(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.
Comment 11 Owen Leonard 2016-11-09 15:57:36 UTC
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.
Comment 12 Josef Moravec 2016-11-09 17:16:44 UTC
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...
Comment 13 Owen Leonard 2016-11-09 17:24:54 UTC
(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.
Comment 14 Josef Moravec 2016-11-10 06:22:14 UTC
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
Comment 15 Josef Moravec 2016-11-10 06:24:25 UTC
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)
Comment 16 Josef Moravec 2016-11-10 06:26:09 UTC
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
Comment 17 Marc Véron 2016-12-16 09:37:26 UTC
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>
Comment 18 Jonathan Druart 2016-12-20 16:23:31 UTC
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>
Comment 19 Kyle M Hall 2016-12-23 11:45:55 UTC
Pushed to master for 17.05, thanks Josef!
Comment 20 Katrin Fischer 2016-12-27 22:18:27 UTC
This won't get ported back to 16.11.x as it is an enhancement.