Bug 10084 - YUI path wrong in CCSR theme with packages
Summary: YUI path wrong in CCSR theme with packages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Packaging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-19 19:25 UTC by Tomás Cohen Arazi
Modified: 2013-12-05 20:09 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10084 - YUI path wrong in CCSR theme with packages (3.09 KB, patch)
2013-04-19 19:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10084 - YUI path wrong in CCSR theme with packages (3.10 KB, patch)
2013-04-20 06:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10084 - YUI path wrong in CCSR theme with packages (3.19 KB, patch)
2013-05-02 04:48 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2013-04-19 19:25:15 UTC
Bug 9052 fixed the PATH for that stuff, but left ccsr without a fix.

To reproduce:
- Install packages from (current) squeeze-dev repo.
- Set CCSR as theme.
- Use Firebug or Chrome's internal debugger to see skin.css and reset-fonts-grids.css are not retreived correctly
- The OPAC doesn't look as awesome as a standard or dev install

To test:
- Apply the patch
- Build your own packages
- Use your koha-common package
- Everything looks ok, no error in debuggers.
Comment 1 Tomás Cohen Arazi 2013-04-19 19:28:47 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2013-04-20 06:31:56 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2013-04-29 15:34:09 UTC
Few questions:
First:
 	install -m 0644 koha-tmpl/opac-tmpl/lib/yui/skin.css \
          koha-tmpl/opac-tmpl/lib/yui/reset-fonts-grids.css \
 		$(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/prog/en/css
+	install -m 0644 koha-tmpl/opac-tmpl/lib/yui/skin.css \
+         koha-tmpl/opac-tmpl/lib/yui/reset-fonts-grids.css \
+		$(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/ccsr/en/css

I have the impression that Koha expects this css to be in lib/yui. So, why install them to prog and ccsr? Better remove the lines before??

Second:
 	sed -i -e 's:url(.*/reset-fonts-grids.css.*):url("/opac-tmpl/lib/yui/reset-fonts-grids/reset-fonts-grids.css"):' \
 		   -e 's:url(.*/skin.css.*):url("/opac-tmpl/lib/yui/assets/skins/sam/skin.css"):' \
-		$(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/prog/en/css/opac.css \
-		$(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/prog/en/css/sco.css
+		$(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/*/en/css/opac.css \
+		$(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/*/en/css/sco.css
I think that this sed should not find anything? I grepped on reset-fonts-grids.css but could not find references to something looking like reset-fonts-grids.css.*  Or is that only in the scope of the install??

Third:
+	sed -i -e 's:href="/opac-tmpl/lib/yui/reset-fonts-grids.css":href="/opac-tmpl/lib/yui/reset-fonts-grids/reset-fonts-grids.css":' \
+		   -e 's:href="/opac-tmpl/lib/yui/skin.css":href="/opac-tmpl/lib/yui/assets/skins/sam/skin.css":' \
+        $(TMP)/usr/share/koha/opac/htdocs/opac-tmpl/ccsr/en/includes/doc-head-close.inc
Why move this skin.css file to assets/skins/sam?
Note that I also did not find the skin.css.* occurrences..

Changing status to reflect need for clarification. After that please switch back to Signed off. Thanks.
Comment 5 Tomás Cohen Arazi 2013-04-29 16:20:38 UTC
Marcel, this is due to the way pacakges handle yui stuff: it relies on yui provided by libyui-js, and thus instead of using the internal one, it deletes it ("scope of the install" in the sense you wrote) and creates a symbolic link to the installed via packages one.

The main problem (and thus the weirdness of the patch, which mimics what is done already) is that the dir structure Koha uses differs from the standard (the one the package provides), and we need to rearrange things.

There should be a patch for arranging yui stuff the same way it is officially arranged, but that needs a lot of rewrite through the code.

Conclusion: this fixes the current problem, in the way we handle this in packages. It should be solved for good in a proper way, in the future.
Comment 6 Marcel de Rooy 2013-05-01 12:03:51 UTC
(In reply to comment #5)
> Marcel, this is due to the way pacakges handle yui stuff

Thanks for your reply. This is not completely clear to me. I switch this back to Signed off. Could Robin qa this kind of patches perhaps?
Comment 7 Tomás Cohen Arazi 2013-05-01 13:30:20 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Marcel, this is due to the way pacakges handle yui stuff
> 
> Thanks for your reply. This is not completely clear to me. I switch this
> back to Signed off. Could Robin qa this kind of patches perhaps?

You could see the fix this way: we already did this for the 'prog' theme, I just added the relevant lines so the same is done for 'ccsr'.

Anyway, I think Robin should take a look too.
Comment 8 Mason James 2013-05-02 04:48:55 UTC
Created attachment 17878 [details] [review]
Bug 10084 - YUI path wrong in CCSR theme with packages

Bug 9052 fixed this for prog, CCSR was left aline in the dark (it's friday).

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Worked fine in my test package, good work Tomas
Signed-off-by: Mason James <mtj@kohaaloha.com>
Comment 9 Mason James 2013-05-02 19:13:54 UTC
(In reply to comment #8)
> Created attachment 17878 [details] [review] [review]
> Bug 10084 - YUI path wrong in CCSR theme with packages
> 
> Bug 9052 fixed this for prog, CCSR was left aline in the dark (it's friday).
> 
> Regards
> To+
> 
> Sponsored-by: Universidad Nacional de Cordoba
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
> 
> Worked fine in my test package, good work Tomas
> Signed-off-by: Mason James <mtj@kohaaloha.com>

passing QA - works as promised
Comment 10 Galen Charlton 2013-05-02 22:20:21 UTC
Pushed to master.  Thanks, Tomás!
Comment 11 Chris Cormack 2013-05-09 20:02:07 UTC
Pushed to 3.10.x will be in 3.10.6