Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE
Summary: Fix layout of Patron Card Creator Layout screen for display in IE
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-13 05:21 UTC by David Cook
Modified: 2014-12-07 20:03 UTC (History)
5 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
Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE (59.98 KB, patch)
2013-02-13 05:58 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE (60.14 KB, patch)
2013-02-13 15:55 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE (13.88 KB, patch)
2013-08-21 02:14 UTC, David Cook
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE (12.95 KB, patch)
2013-08-26 12:39 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE (13.26 KB, patch)
2013-08-27 21:14 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2013-02-13 05:21:08 UTC
Currently, the layout for the Edit/Add Layout screen of the Patron Card Creator is in complete disarray, when viewed in Internet Explorer (of any version). 

The nav bar is pushed to the bottom of the page, every fieldset is empty (as their contents have been pushed out into different parents of the page), and the checkboxes don't work.

The cause appears to be some extraneous mark-up (a few extra fieldset and li elements)and some missing mark-up (ol elements to wrap the list elements, especially when nesting lists within each other and within div elements).
Comment 1 David Cook 2013-02-13 05:58:20 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-02-13 15:55:34 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-03-26 15:00:13 UTC
(In reply to comment #2)
> I don't like all the whitespace changes in this patch. Making whitespace
> changes as well as code/markup changes is generally speaking a violation
> of coding guidelines. However, this is not a frequently-modified
> template and a diff which ignores whitespace changes shows that the real
> changes are minor and effective.
> 
> I'm signing off and correcting the commit message to include a
> description of the fix and the test plan.

Do not like these whitespace changes too. Could the author resolve this please and make life easier for QA? Thanks!
Comment 4 David Cook 2013-08-21 01:06:36 UTC
(In reply to M. de Rooy from comment #3)
> (In reply to comment #2)
> > I don't like all the whitespace changes in this patch. Making whitespace
> > changes as well as code/markup changes is generally speaking a violation
> > of coding guidelines. However, this is not a frequently-modified
> > template and a diff which ignores whitespace changes shows that the real
> > changes are minor and effective.
> > 
> > I'm signing off and correcting the commit message to include a
> > description of the fix and the test plan.
> 
> Do not like these whitespace changes too. Could the author resolve this
> please and make life easier for QA? Thanks!

If you use the -w flag to not show whitespace changes, you should be all right. 

However, I'm working on this patch again locally today, so I'll look at uploading a new version with code changes in one patch and whitespace changes in the other.
Comment 5 David Cook 2013-08-21 02:14:52 UTC Comment hidden (obsolete)
Comment 6 David Cook 2013-08-21 02:17:40 UTC
Actually, I'm not sure that it's necessary to add a patch for the whitespace.

I will if folks think it's a good idea, but this is the essence of the patch.

P.S. It looks like I forgot to include Owen's sign off in this latest patch. Could QA folks add that back in if necessary?
Comment 7 Owen Leonard 2013-08-26 12:39:56 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2013-08-27 21:14:43 UTC
Created attachment 20672 [details] [review]
[PASSED QA] Bug 9603 - Fix layout of Patron Card Creator Layout screen for display in IE

Currently, the layout for the Edit/Add Layout screen of the Patron
Card Creator is in complete disarray, when viewed in Internet Explorer
(of any version).

The nav bar is pushed to the bottom of the page, every fieldset is
empty (as their contents have been pushed out into different parts
of the page), and the checkboxes don't work.

The cause appears to be some extraneous mark-up (a few extra fieldset
and li elements)and some missing mark-up (ol elements to wrap the list
elements, especially when nesting lists within each other and within
div elements).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
I didn't test this in IE, but changes are fixing general
problems with HTML validity.

Checking the page with the W3C validator:
Before: 27 Errors, 7 warning(s)
After:  6 Errors, 7 warning(s)

All tests and QA script pass, page looks alright.
Would be nice to see the remaining problems fixed in
a follow-up.
Comment 9 Galen Charlton 2013-09-04 18:16:34 UTC
Pushed to master -- thanks, David!
Comment 10 Tomás Cohen Arazi 2013-09-13 18:47:05 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks David!
Comment 11 Bernardo Gonzalez Kriegel 2013-10-13 13:27:38 UTC
Pushed to 3.10.x, will be in 3.10.12