Created attachment 45249 [details]
Screenshot of updated layout

Sorry, the changes were just for commit message formatting.

My initial reaction is that this looks really great. I'm going to test, but I want to point out that this bug has no test plan where it really needs a very vigorous one. There are a lot of OPAC settings which need to be tested on and off, and with XSLT turned on and off.

(In reply to Owen Leonard from comment #8)
> My initial reaction is that this looks really great. I'm going to test, but
> I want to point out that this bug has no test plan where it really needs a
> very vigorous one. There are a lot of OPAC settings which need to be tested
> on and off, and with XSLT turned on and off.

You're right Owen.

Nice work Jen, the screen layout looks much better.

Hi Jen

Average rating disappear when there not exists summary in record. Test with another book without summary (520)

Good catch, Héctor. I had tried to make it agnostic to the summary being there but it's a bit tricky. I will see if I can sort that out.

Owen, regarding the test plan, my apologies it doesn't seem to be in the developer workflow on the wiki (this is my first proper Koha patch). Any chance you could point me at something on the wiki, or another bug I could look at as an example to see what that would look like? And then I can sort that out and get it added. Is it just like the "to test bit" in the commit message on the commit message page on the wiki? Like you say, I think a formal test plan for this would be a lot more complicated than that example, would the commit message be the right place for it?

Hi, I've updated the patch to make the logic for moving the ratings controls upwards a lot more robust and agnostic of the presence of particular fields. I've also added a crude test plan. I'm _not_ a Koha expert and it might be missing some necessary detail about specific configurations of fields that I'm unaware of, but hopefully it's a good starting place.

Hi Jen

I think (IMHO) if you can divide in more divs the details, i.e., all data is contained in <div id="detail" class="span10"> and it could look much better if you separate in two or three divs depends on the data displayed. If record is too long -like motion pictures, dvds, etc- a white block is displayed below the cover image, so this way the second div can be aligned at left and so on. Here we must consider that two or more covers can be deployed. Also Koha notice this in Enhanced Content preferences in All section "NOTE: you can only choose one source of cover images from below, otherwise Koha will show the images from all sources selected."

If OPACLocalCoverImages is set to 'Display' and there is not a cover image <div class="span2"> must be hide or display something like "No cover image available" in a box.

I can not have access to Enhanced Content that require account and password like NoveList Select, If you have access, please paste a link for video like Kyle Hall did it in bug <http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15079>. I don't know if QA Team have access to this.

I've been tested this with syprefs GoogleJackets, OpenLibraryCovers, OPACAmazonCoverImages and works excellent, I will test it with more syprefs involved.

Could be great if you extend this to UNIMARC and NORMARC environment.

Regards

Hi Héctor,

Thanks again for you work looking into this. It looks like the local cover images not having a proper placeholder is a bug as a 1x1 pixel image is returned. There is a bug that addresses this at: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11602

I'm aware that there is whitespace under the cover image (or images if you have more than one source turned on). This is by design, IMO it makes the page look less cluttered. Having a narrower column for the main details is not necessarily a bad thing, there are studies that say that slightly narrower columns are easier to read (see e.g. http://graphicdesign.stackexchange.com/questions/13724/recommended-column-width-for-text-reading-digital-vs-printed).

I had a look at porting some of the changes to NOMARC and UNIMARC but I'm a bit leery of messing with them as I'm not sure I'm grokking some of the data structures. I could help with porting the changes from this patch to those systems as a later patchset. In the meantime, those systems will still get some but not all of the changes from this patch: the cover image column, larger font size for the authors, ratings controls moved up under the authors. They won't get the item summary moved up and the larger font sizes for series, summary and item type.

I have made one change in the most recent update. The 773 data in MARC21slim2OPACDetail.xsl is moved back down to its original location. I had moved it up along with the summary field, but I don't think any more that they need to move together, and leaving it in its original position reduces the scope of this change.

I don't unfortunately have any credentials either, but I am checking with my colleagues if we can shoulder tap someone who does to do a bit more testing on that front. 

Thanks,

Jen

You're right Jen if you apply 11602 on top the placeholder is fixed it up.

For me status need to change to Needs sign-off

Tested with all non-subscription cover sources and novelist, all looks really good!

Owen, could you have a look at this one please?

I really would like to get another signoff on this one.

Created attachment 46885 [details] [review]
[SIGNED-OFF] Bug 15269 [ENH]: Layout tweaks for the opac-detail page

Make the following improvements to the layout of
the OPAC detail page:

* Image thumbnail has more white space
* Summary is moved up the details block and font
  size is increased
* Ratings controls are moved up the details
* Small devices get a better layout

To test:
- Check that the ratings controls are moved up the
  page to be before the summary
    * If there is no summary, the controls should
      be under the series
    * If there is no summary or no series, the
      controls should be under the type
    * If there is no summary, no series, or no type,
      the controls should be under the authors
      (including contributing authors)
- Check that the 520 field (Summary, Subject, Review,
  Scope and content, Abstract, Content advice) is
  rendered nearer to the top of the page, following
  material type, series, and the ratings.
- Check that the font size for "Material type",
  "Series" and the 520 field (Summary, Subject,
  Review, Scope and content, Abstract, Content advice)
  are rendered at a 14px font size.
- Check that authors and contributing authors are
  rendered at 16px font size.
- Check that if any kind of cover image is present
  (or the no cover image placeholder image) it is
  rendered into a div with a class of 'span2' and
  appears on the left of the main item details.
    * Need to test every kind of cover image service
    * If no kind of cover image service is turned on,
      then the 'span2' div should not be present and
      the item details should be on the left of the
      content area.
- Check that small devices (e.g. mobile phones) make
  the 'span2' image column take up the full width and
  appear above the rest of the item details.
- Check that layout of the search results page is
  unaffected.
- Check that changing 'OPACXSLTDetailsDisplay' to an
  empty string still yields the expected layout.

Sponsored-by: ByWater

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 46886 [details] [review]
[SIGNED-OFF] Bug 15269 [ENH]: Compiled CSS changes

Sponsored-by: ByWater

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

If JS is disabled, the .span2 is empty but takes a lot of space (even with small devices).
If JS is enabled and you don't have a local cover image (and pref OPACLocalCoverImages is set), the space takes by the span2 is much more wide than the "no cover" image.

I'd say it's not blocker, but I'd prefer someone confirm it's the expected behavior.

Hi Jonathan,

Yep, that should probably be fixed we'll take a look into a follow up for that.

Cheers,
Liz

When trying to apply patch I am present with the following error:
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt

I tried to resolve the conflict that occurs while applying this patch, the conflict is very big and confusing