Bug 11885 - Inconsistent HTML in MARC Details
Summary: Inconsistent HTML in MARC Details
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: paxed
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-04 07:37 UTC by paxed
Modified: 2018-08-28 11:18 UTC (History)
6 users (show)

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


Attachments
Make MARC Details tab HTML consistent (22.18 KB, patch)
2014-03-04 07:37 UTC, paxed
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11885 - Inconsistent HTML in MARC Details (22.40 KB, patch)
2014-03-19 16:24 UTC, Mark Tompsett
Details | Diff | Splinter Review
with patch (10.01 KB, image/png)
2014-03-20 08:53 UTC, Jonathan Druart
Details
without patch (9.95 KB, image/png)
2014-03-20 08:54 UTC, Jonathan Druart
Details
Bug 11885 - Follow-up to address issues raised (3.21 KB, patch)
2014-04-03 00:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11885 - Follow-up to address issues raised (3.25 KB, patch)
2014-04-07 04:42 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11885 - Inconsistent HTML in MARC Details (22.46 KB, patch)
2014-04-09 11:40 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11885 - Follow-up to address issues raised (3.38 KB, patch)
2014-04-09 11:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 11885 - followup to remove tabs (2.62 KB, patch)
2014-04-09 11:45 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 11885 - Inconsistent HTML in MARC Details (22.51 KB, patch)
2014-04-11 08:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11885 - Follow-up to address issues raised (3.44 KB, patch)
2014-04-11 08:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11885 - followup to remove tabs (2.71 KB, patch)
2014-04-11 08:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11885: Add a css class to set subfield code bold (2.03 KB, patch)
2014-04-11 08:39 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 paxed 2014-03-04 07:37:51 UTC Comment hidden (obsolete)
Comment 1 Mark Tompsett 2014-03-19 16:24:05 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-03-20 08:53:32 UTC
QA comment:

I didn't find any regression (tag missing, etc.) but 3 smalls things differ from master:
1/ subtags was bold
2/ 1 dash existed between tag and tag name
3/ A space has been added ("606 #1 - Sujet nom commun" becomes "606 # 1 Sujet nom commun",  "101 ## - Langue" becomes "101 # # Langue")

I am going to provide screenshots.

Marked as Failed QA.
Comment 3 Jonathan Druart 2014-03-20 08:53:59 UTC
Created attachment 26468 [details]
with patch
Comment 4 Jonathan Druart 2014-03-20 08:54:20 UTC
Created attachment 26469 [details]
without patch
Comment 5 paxed 2014-03-20 09:19:53 UTC
(In reply to Jonathan Druart from comment #2)
> QA comment:
> 
> I didn't find any regression (tag missing, etc.) but 3 smalls things differ
> from master:
> 1/ subtags was bold
> 2/ 1 dash existed between tag and tag name
> 3/ A space has been added ("606 #1 - Sujet nom commun" becomes "606 # 1
> Sujet nom commun",  "101 ## - Langue" becomes "101 # # Langue")
> 
> I am going to provide screenshots.
> 
> Marked as Failed QA.

If you really want the exact same behaviour as before, add the following CSS to your IntranetUserCSS:

.tag_title .tag_desc:before { content: " - "; }
.tag_title .tag_ind2 { margin-left:-4px; }
.subfield-label { font-weight: bold; }
div[id=tab0XX] .subfield-label,
.subfield-label .desc { font-weight: normal; }
Comment 6 Jonathan Druart 2014-03-20 09:29:44 UTC
(In reply to paxed from comment #5)
> If you really want the exact same behaviour as before, add the following CSS
> to your IntranetUserCSS:

I don't think it is the good answer :) Was it expected? If yes, why?
Comment 7 Mark Tompsett 2014-04-03 00:36:16 UTC Comment hidden (obsolete)
Comment 8 paxed 2014-04-03 04:33:42 UTC
(In reply to M. Tompsett from comment #7)
> Created attachment 26773 [details] [review] [review]
> Bug 11885 - Follow-up to address issues raised
> 
> This patch addresses them in the following way:
> 1/ You will note that @ was not bold on the 0 tab.
>    Every other tab were bold. By making the similar template
>    into a procedure based on the 0XX tab, bolding was lost.
>    This patch bolds all subtags including the @, so that
>    the visible change is minimized.

Why hard-coded <b> tags? Isn't there some good reason to separate presentation and content?


> 2/ The dash was programmatically added in at the code stage
>    previously. This bug fix splits the the single concatenation
>    mess into parts which can be styled. This puts the dash back
>    into the template. However, it should be noted that the
>    spacing for the 0 tab's tag and tag description will have an
>    extra space after the hypen that was lacking before.

Wrap the dash and the spaces before the description in <span class="dashwrapper"> or something, so it'll be easier to hide with css.


> 3/ <span>...</span><span>...</span> is different than
>    <span>...</span>
>    <span>...</span>
>    The later puts that extra space. This patch fixes that.

This one is good.
Comment 9 Mark Tompsett 2014-04-07 03:37:42 UTC
(In reply to paxed from comment #8)
> (In reply to M. Tompsett from comment #7)
> > Created attachment 26773 [details] [review] [review] [review]
> > Bug 11885 - Follow-up to address issues raised
> > 
> > This patch addresses them in the following way:
> > 1/ ...
> 
> Why hard-coded <b> tags? Isn't there some good reason to separate
> presentation and content?

Because that is the way it was done before. I'm not saying it was the best way to do it, but it reduces visible differences.


> > 2/ ...
>
> Wrap the dash and the spaces before the description in <span
> class="dashwrapper"> or something, so it'll be easier to hide with css.

Sure, why not.
Comment 10 Mark Tompsett 2014-04-07 04:42:38 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2014-04-09 11:40:57 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2014-04-09 11:41:11 UTC Comment hidden (obsolete)
Comment 13 Bernardo Gonzalez Kriegel 2014-04-09 11:45:04 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-04-11 08:38:44 UTC
Created attachment 26992 [details] [review]
Bug 11885 - Inconsistent HTML in MARC Details

In Catalog > MARC Details, the HTML in the different tabs is slightly
inconsistent and doesn't differentiate different elements, making CSS
styling complicated or impossible:

* tab 0 has <p class="subfield_line"> whereas all the other tabs
  have just <p>

* all other tabs wrap the subfield character in <b> tags, except
  for tab 0

* the MARC tag title is a single div with the tag, the indicators
  and the field description.

Attached patch folds all the tab outputs into a single TT BLOCK,
which is then reused. It also marks the separate parts of the tag
title in their own spans.
The output should be nearly identical to previous behaviour, minus
a dash from the tag title descriptions - it was used to separate
the tag from the description. The description can now be styled
separately from the tag itself, so the dash can be added with CSS,
if necessary.

Revised test:
1) Find a biblio
2) Edit the record so that there is something in every tab (0-9).
3) Save and then click 'MARC' in the left pane to view the
    MARC details.
4) Note the contents of each tab.
5) Apply patch
6) Compare the MARC details output to what was noted. Should
   be the same, minus a dash in each of the tag title descriptions.

http://bugs.koha-community.org/show_bug.cgi?id=11885

Signed-off-by: Pasi Kallinen <pasi.kallinen@pttk.fi>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 15 Jonathan Druart 2014-04-11 08:38:56 UTC
Created attachment 26993 [details] [review]
Bug 11885 - Follow-up to address issues raised

Jonathan Druart raised the following three issues:
1/ subtags was bold before patch
2/ 1 dash existed between tag and tag name
3/ A space has been added ("606 #1 - Sujet nom commun" becomes "606 # 1 Sujet nom commun",  "101 ## - Langue" becomes "101 # # Langue")

This patch addresses them in the following way:
1/ You will note that @ was not bold on the 0 tab.
   Every other tab were bold. By making the similar template
   into a procedure based on the 0XX tab, bolding was lost.
   This patch bolds all subtags including the @, so that
   the visible change is minimized.
2/ The dash was programmatically added in at the code stage
   previously. This bug fix splits the the single concatenation
   mess into parts which can be styled. This puts the dash back
   into the template. However, it should be noted that the
   spacing for the 0 tab's tag and tag description will have an
   extra space after the hypen that was lacking before.
3/ <span>...</span><span>...</span> is different than
   <span>...</span>
   <span>...</span>
   The later puts that extra space. This patch fixes that.

See comment 1 for the test plan.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Work as described.
Small koha-qa errors fixed in followup

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 16 Jonathan Druart 2014-04-11 08:39:08 UTC
Created attachment 26994 [details] [review]
Bug 11885 - followup to remove tabs

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 17 Jonathan Druart 2014-04-11 08:39:40 UTC
Created attachment 26995 [details] [review]
Bug 11885: Add a css class to set subfield code bold

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 18 Jonathan Druart 2014-04-11 08:41:15 UTC
All looks good, no regression found.
Marked as Passed QA.
Comment 19 Galen Charlton 2014-04-11 16:28:12 UTC
Pushed to master. Thanks, Pasi!
Comment 20 Fridolin Somers 2014-08-06 07:54:10 UTC
Pushed to 3.14.x, will be in 3.14.10

(code real improvement, nice job)
Comment 21 Kyle M Hall 2014-08-13 13:49:35 UTC
Picked for 3.12.15