Bug 11881

Summary: Small improvements in OPAC results list
Product: Koha Reporter: Frédérick Capovilla <fcapovilla>
Component: OPACAssignee: simith.doliveira
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: eric.begin, francois.charbonnier, genevieve.plantin, jonathan.druart, katrin.fischer, mtompset, nengard, nick, oleonard, philippe.blouin, simith.doliveira, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Small improvements for the availability informations in the search results list on the OPAC.
New version of the patch. Rebase on current master.
Small improvements for the availability informations in the search results list on the OPAC.
Bug 11881 - Follow-up: Add CSS entries to opac.css (prog)
Bug 11881: (Follow-up) Add the CommasWithLinebreaks and LabelCallNumber classes to Bootstrap them and fix the Call number typo
Bug 11881: (Follow-up) Add the CommasWithLinebreaks and LabelCallNumber classes to Bootstrap them and fix the Call number typo
Bug 11881: (Follow-up) Add the CommasWithLinebreaks and LabelCallNumber classes to Bootstrap them and fix the Call number typo
Small improvements for the availability informations in the search results list on the OPAC.
[SIGNED-OFF] Small improvements for the availability informations in the search results list on the OPAC.
Add changes to opac.less
Small improvements for the availability informations in the search results list on the OPAC
Small improvements for the availability informations in the search results list on the OPAC
Bug 11881 - Small improvements for Items available for reference
Small improvements for the availability informations in the search results list on the OPAC
Bug 11881 - Small improvements for Items available for reference
Bug 11881: Small improvements for the availability informations in the search results list on the OPAC
Bug 11881 - Small improvements for Items available for reference
[PASSED QA] Bug 11881: Small improvements for the availability informations in the search results list on the OPAC
[OPTIONAL] Bug 11881: QA follow-up - rename class
[PASSED QA] Bug 11881: Small improvements for the availability informations in the search results list on the OPAC
[PASSED QA] Bug 11881 - Small improvements for Items available for reference
[OPTIONAL] Bug 11881: QA follow-up - rename class

Description Frédérick Capovilla 2014-02-28 22:00:08 UTC Comment hidden (obsolete)
Comment 1 Mark Tompsett 2014-03-02 05:28:24 UTC
There's a merge problem. I'll let the original author rebase before I sign off, rather than me sign off on my rebase of it.
Comment 2 Katrin Fischer 2014-03-02 08:31:42 UTC
I think this change could be something not everyone will agree to as it sounds like the items line will take up a lot more space for bigger consortia with lots of libraries.
Comment 3 Mark Tompsett 2014-03-02 15:12:25 UTC
Yes, the substitution of a comma for a line break does increase vertical spacing. However, I personally find it more visually appealing and less of a giant blur from one into another. The vertical spacing increases *IF* there are multiple branches with the same biblio. A single branch would not experience any visual difference.
Comment 4 Owen Leonard 2014-03-03 18:37:38 UTC
This sounds like the kind of change that could be better handled by using a custom XSL stylesheet.
Comment 5 Mark Tompsett 2014-03-03 18:48:26 UTC
You're probably right, Owen. Though, you'd have to keep re-patching, if the default templates shifted for security reasons. Which is probably why they want it in the default.
Comment 6 Katrin Fischer 2014-03-03 19:03:22 UTC
There are system preferences for defining your own custom XSLT paths to be used for OPAC and staff, so repatching won't be needed. Of course, you will have to add new features to your custom files or add your changes to a copy of the default again, but updates won't overwrite your changes.
Comment 7 Frédérick Capovilla 2014-03-06 22:33:58 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2014-03-07 05:44:48 UTC
Changes to koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl
2008:      13
2009:      14
2010:      14
2011:      10
2012:       9
2013:       6
2014:       1

It would seem to me that the amount the template is changing is going down, but with it changing every 1-2 months, I can understand why the change is desired in master.

Is there a way to system preference the line break vs. comma in XSLT? Perhaps that would be more acceptable, and default to comma given that is the current behaviour.

I changed this to in discussion, because as far as I can tell this patch won't make it past QA, because of the comma vs. line break difference and because perhaps a system preference thought may save this desired line break functionality.
Comment 9 Owen Leonard 2014-03-10 14:36:27 UTC
QA issues:

- The patch only changes prog, not bootstrap
- "Call Number" is incorrectly capitalized. 

If we want to avoid custom XSLT the compromise would be to add markup and classes to the XSL so that the these snippets of text can be styled and/or hidden as desired:

- Wrap the individual library lines in a span which can be made 
  "display:block" in custom CSS.

- Wrap the "cal number" text in a span which is hidden by default in the CSS.
  Use custom CSS to show it.
Comment 10 Mark Tompsett 2014-07-14 15:27:35 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2014-07-14 15:27:47 UTC Comment hidden (obsolete)
Comment 12 Eric Bégin 2014-07-18 14:53:46 UTC
Mark, did you forget to set this bugs as « Needs Signoff » ?
Comment 13 Mark Tompsett 2014-07-18 15:00:29 UTC
No, because of comment 9 requiring a bootstrap portion of the patch. I just fixed the prog side.
Comment 14 Eric Bégin 2014-07-19 10:36:35 UTC Comment hidden (obsolete)
Comment 15 Eric Bégin 2014-07-19 10:41:04 UTC
Comment on attachment 29873 [details] [review]
Bug 11881: (Follow-up) Add the CommasWithLinebreaks and LabelCallNumber classes to Bootstrap them and fix the Call number typo

Another patch will follow with all required files
Comment 16 Eric Bégin 2014-07-19 10:52:49 UTC Comment hidden (obsolete)
Comment 17 Eric Bégin 2014-07-19 10:55:03 UTC
Bootstrap is now supported
Comment 18 Mark Tompsett 2014-07-19 13:05:59 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2014-08-08 18:11:24 UTC
Patch doesn't apply

Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/less/opac.less
Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/opac.css
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/css/opac.css
Failed to merge in the changes.
Patch failed at 0001 Bug 11881: (Follow-up) Add the CommasWithLinebreaks and LabelCallNumber classes to Bootstrap them and fix the Call number typo
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-11881-Follow-up-Add-the-CommasWithLinebreaks-a-aaGWGn.patch
Comment 20 simith.doliveira 2014-11-27 20:43:26 UTC Comment hidden (obsolete)
Comment 21 simith.doliveira 2014-11-27 20:44:45 UTC
To test:

I) Apply the patch
validate: no breakline nor "Call Number:" text.

II) Include the following test in the OPACUserCSS preference:

.searchresults .CommasWithLinebreaks {
    display: block; # block - line breaks, inline - all one line
}

.searchresults .LabelCallNumber {
    display: inline; # inline - display, none - don't display
}

validate: Breakline or "Call Number:" text.
Comment 22 Nicole C. Engard 2015-03-04 12:02:33 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-03-04 12:54:39 UTC
Simith,
The css file is generated from the less file (less/opac.less).
You need to attach to the patch the change you have done to this file too.
Comment 24 simith.doliveira 2015-03-04 14:11:27 UTC Comment hidden (obsolete)
Comment 25 simith.doliveira 2015-03-04 14:12:25 UTC
opac.less file added
Comment 26 Jonathan Druart 2015-03-04 15:04:35 UTC
(In reply to simith.doliveira from comment #25)
> opac.less file added

Ok thanks.
No signoff needed for this one.
Comment 27 Jonathan Druart 2015-03-04 15:26:49 UTC
The opac.css does not look good.
With your patch:
-rw-r--r-- 1 koha koha 42132 Mar  4 16:25 bootstrap/css/opac.css

$lessc --compress bootstrap/less/opac.less > bootstrap/css/opac.css
-rw-r--r-- 1 koha koha 42946 Mar  4 16:25 bootstrap/css/opac.css
Comment 28 simith.doliveira 2015-03-04 19:12:03 UTC Comment hidden (obsolete)
Comment 29 simith.doliveira 2015-03-04 19:22:50 UTC
I created a new patch with just MARC21slim2OPACResults.xsl file because reading the comments #2, #4, #9 and #21 we don't need .css ou .less files.
Comment 30 Mark Tompsett 2015-03-05 05:01:57 UTC
(In reply to Jonathan Druart from comment #27)
> The opac.css does not look good.
> With your patch:
> -rw-r--r-- 1 koha koha 42132 Mar  4 16:25 bootstrap/css/opac.css
> 
> $lessc --compress bootstrap/less/opac.less > bootstrap/css/opac.css
> -rw-r--r-- 1 koha koha 42946 Mar  4 16:25 bootstrap/css/opac.css

mtompset@ubuntu:~/kohaclone$ git reset --hard origin/master                HEAD is now at 45e5afb Bug 13731: Suggestions reason does not save on updating the status
mtompset@ubuntu:~/kohaclone$ ls -la koha-tmpl/opac-tmpl/bootstrap/css/opac.css
-rw-rw-r-- 1 mtompset mtompset 42879 Mar  5 00:01 koha-tmpl/opac-tmpl/bootstrap/css/opac.css
mtompset@ubuntu:~/kohaclone$ lessc --compress koha-tmpl/opac-tmpl/bootstrap/less/opac.less > koha-tmpl/opac-tmpl/bootstrap/css/opac.css
mtompset@ubuntu:~/kohaclone$ ls -la koha-tmpl/opac-tmpl/bootstrap/css/opac.css
-rw-rw-r-- 1 mtompset mtompset 43371 Mar  5 00:01 koha-tmpl/opac-tmpl/bootstrap/css/opac.css
mtompset@ubuntu:~/kohaclone$

I don't think it is this patch per say.
Comment 31 Katrin Fischer 2015-04-06 18:17:06 UTC
I have stared at this for a while, there are some things I am not completely happy with:

1) There is a visible change on the result list with this patch - the added 'Call number: ' in front of every entry. This can be hidden with CSS, so I think it's probably ok.

Example:
Availability: Items available for loan: Fairfield [Call number: 320] (1), Franklin [Call number: 320] (1), Riverside [Call number: 320] (1), Union [Call number: 320] (1). 

2) I am stumbling a bit over 'CommasWithLinebreaks' as a class name. I'd like something that describes the content better.

3) The 'Call number: ' change is not consistent. If you have an item whose item type is marked as not for loan, it will show up separately:

Example: Availability: Items available for loan: Franklin [Call number: 320] (1), Riverside [Call number: 320] (1), Union [Call number: 320] (1). Items available for reference: Fairfield [320] (1 ). 

Failing for 3) - would be good to get another opinion on 1) and 2).
Comment 32 Mark Tompsett 2015-04-06 22:24:17 UTC
Katrin, you'll note the obsoleted "Add changes to opac.less" file which an be used to build tweaks to the opac.css file which would hide the label.

So, I don't think (1) is an issue, except that perhaps the less and corresponding css change shouldn't be obsoleted?

CommasWithLinebreaks is EXACTLY what it can be used for. With the appropriate CSS, Linebreaks between the items can be forced. I don't think the name is so far off what it actually is intended to do. So, I don't see an issue with (2).

However, you rightly point out (3) is inconsistent. I agree that should be fixed before signing off.
Comment 33 Genevieve Plantin 2015-09-11 18:58:41 UTC Comment hidden (obsolete)
Comment 34 Genevieve Plantin 2015-09-11 18:58:55 UTC Comment hidden (obsolete)
Comment 35 Nick Clemens 2015-09-11 20:36:07 UTC Comment hidden (obsolete)
Comment 36 Nick Clemens 2015-09-11 20:36:31 UTC Comment hidden (obsolete)
Comment 37 Nick Clemens 2015-09-11 20:37:43 UTC
One small thing I noticed was an extra line between the available items list and the Checked Out(1) line after I had applied the CSS snippet
Comment 38 Genevieve Plantin 2015-09-14 12:17:41 UTC
(In reply to Nick Clemens from comment #37)
> One small thing I noticed was an extra line between the available items list
> and the Checked Out(1) line after I had applied the CSS snippet

Is 'Checked out' supposed to be on the same line as an [Call number : 320] ? I just want to be sure I correct it the good way. I was thinking about it because there's other unavailable classes like Withdrawn, Lost, Damaged, etc. Are they supposed to be on the same line too ? Wouldn't it weird if it was only in the same line as one Call Number even if there is three Call Number ?
Comment 39 Jonathan Druart 2015-09-23 13:51:33 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2015-09-23 13:51:38 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2015-09-23 13:53:19 UTC
Looks fine by me.
I am waiting for a confirmation from Katrin before passing QA on it.
Comment 42 Katrin Fischer 2015-10-25 10:45:12 UTC
No problem with the patch itself, but I am not a fan of the name of the class:
CommasWithLinebreaks
A good rule is not to name classes on appearance, as appearance can change with CSS, but according to what they contain - something that describes that it's the item information for one branch maybe.
Comment 43 Katrin Fischer 2015-10-25 10:54:14 UTC Comment hidden (obsolete)
Comment 44 Katrin Fischer 2015-10-25 10:57:05 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2015-10-25 10:57:48 UTC
Created attachment 43948 [details] [review]
[PASSED QA] Bug 11881: Small improvements for the availability informations in the search results list on the OPAC

This patch contains small modifications to the template of OPAC search results.
Theses modifications affect the "Availability" line :
* Add a linebreak after each location.
* Add the text "Call Number:" before call numbers.

To test:

I) Apply the patch

II) Include the following test in the OPACUserCSS preference:

.searchresults .CommasWithLinebreaks {
    display: block; # block - line breaks, inline - all one line
}

.searchresults .LabelCallNumber {
    display: inline; # inline - display, none - don't display
}

III) Do a search on the OPAC

IV) Validate : check the format of availability informations for each search result (Breakline or "Call Number:" text).

Sponsored-by: CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 46 Katrin Fischer 2015-10-25 10:57:51 UTC
Created attachment 43949 [details] [review]
[PASSED QA] Bug 11881 - Small improvements for Items available for reference

As said on comment #31, Items available for reference weren't displayed like Items avaible for loan. Now, they are, I wrapped them in CommasWithLinebreaks class and LabelCallNumber class

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Katrin Fischer 2015-10-25 10:57:55 UTC
Created attachment 43950 [details] [review]
[OPTIONAL] Bug 11881: QA follow-up - rename class

A suggestion for renaming the class to be a bit more "content" then "appearance".
Strictly optional.

.ItemSummary {
    display: block; # block - line breaks, inline - all one line
}

.LabelCallNumber {
    display: inline; # inline - display, none - don't display
}
Comment 48 Tomás Cohen Arazi 2015-10-29 17:25:35 UTC
Patches pushed to master.

Thanks Simith and Genevieve!