a syspref to add 'powered by Koha' text to OPAC footer
Created attachment 4747 [details] [review] patch
Mason, I have some questions: #1 where will this appear - above or below opacfooter? #2 why not allow for choosing to show the koha logo or the text? Nicole
patch applied to commit 68f99cdf980222dcb557667925bd6024640cda2a
Created attachment 4748 [details] [review] a little formatting tweak...
Created attachment 4749 [details] screenshot
(In reply to comment #2) > Mason, > > I have some questions: > > #1 where will this appear - above or below opacfooter? below - take a look at my screenshot > #2 why not allow for choosing to show the koha logo or the text? that will be the next patch, after this patch is accepted ;)
Hi Mason, I like this idea! I tested with the new syspref off/on and languagechooser off/on. Some things are missing from the patch: - updatedatabase for existing installatons - translates systempreferences sql files I am marking failed QA because of those 2 problems. I also have some suggestions: - make the link open in a new window - make 'powered by Koha' a link, instead of only Koha I am not sure about having the css directly in the anchor tag. This way it's hard to customize it, like changing the font size or adding a picture. Perhaps having another spot for cutomizable HTML code (like opaccredits) with a default value would work too?
> Some things are missing from the patch: > > - updatedatabase for existing installatons fyi: an updatedatabase.pl entry is not needed here - the default behaviour for this feature is OFF. when the feature set to ON, the 'OpacKohaUrl' row is created in the systempreferences table so... no updatedatabase.pl is needed ;) > - translates systempreferences sql files my new patch adds these...
Created attachment 4762 [details] [review] new patch now with row added to all sysprefs.sql
applied to patch d4b397feb41fe6b0cb8672d8c002b1b0f709b097 http://git.kohaaloha.com/?p=head-dev/.git;a=commit;h=a8f0a96f7cda17fb6cb00460b4166c9fe697e262
(In reply to comment #7) > Hi Mason, > > I like this idea! > I also have some suggestions: > - make 'powered by Koha' a link, instead of only Koha my patch does this already... > > I am not sure about having the css directly in the anchor tag. This way it's > hard to customize it, like changing the font size or adding a picture. > Perhaps having another spot for cutomizable HTML code (like opaccredits) with a > default value would work too? these are great ideas, lets do them after this patch gets accepted...
I brought this up on IRC, because I was not sure about the updatedatabase. Like you said it will be added to the systempreferences table on first change, but consensus was that updating the database makes sense for a couple of reasons: (09:53:02) paul_p: kf : I agree with rangi = the problem with "no updatedatabase for syspref" is also that it's not created until you explicitly save something. It will work, but may cause trouble sometimes (SQL request to see what's the value of the syspref for example) (09:47:16) rangi: he is right, the way the code works, if it is in the .pref file, it will get created when changed, but (09:47:40) rangi: the update is good because it tells people a new syspref has been added
Created attachment 4784 [details] [review] revised patch, with updatedatabase.pl SQL added. (In reply to comment #12) > I brought this up on IRC... aaah, those are all good reasons. here is a revised patch, with updatedatabase.pl SQL added. thanks all for the feedback :)
I am very sorry, but your patch is changing kohaversion.pl If you are adding a syspref, please add a section with subversion XXX to updatedatabase.pl. The RM updates kohaversion.pl later on.
Created attachment 4850 [details] [review] revised patch, with koha-version.pl edit removed
(In reply to comment #14) > I am very sorry, but your patch is changing kohaversion.pl this is now corrected in a newer patch > If you are adding a syspref, please add a section with subversion XXX to > updatedatabase.pl. this is now corrected in a newer patch > The RM updates kohaversion.pl later on. ok, thanks for this info
-------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: add 'powered by Koha' text to OPAC footer Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Auth.pm Auto-merging installer/data/mysql/de-DE/mandatory/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/mandatory/sysprefs.sql Auto-merging installer/data/mysql/en/mandatory/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sysprefs.sql Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql Auto-merging installer/data/mysql/it-IT/necessari/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/it-IT/necessari/sysprefs.sql Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql Auto-merging installer/data/mysql/pl-PL/mandatory/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/sysprefs.sql Auto-merging installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql CONFLICT (content): Merge conflict in installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql Auto-merging installer/data/mysql/uk-UA/mandatory/system_preferences_full_optimal_for_install_only.sql CONFLICT (content): Merge conflict in installer/data/mysql/uk-UA/mandatory/system_preferences_full_optimal_for_install_only.sql Auto-merging installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref Failed to merge in the changes. Patch failed at 0001 add 'powered by Koha' text to OPAC footer When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". nengard@debian:~/kohaclone$
Created attachment 4873 [details] [review] revised patch
(In reply to comment #18) > Created attachment 4873 [details] [review] > revised patch applied to commit 240edb6c0da80d3d932442ccd96f9f98e31260c3 '5660 Webinstaller should check if KohaOpacLanguage exists in staff'
(In reply to comment #18) > Created attachment 4873 [details] [review] > revised patch patch emailed to patches@kc.org
Owen, I added you to the CC list of this report. Patch looks good to me, except from the CSS related issues on the Powered by Koha footer. I guess it would be better to move the hardcoded style stuff to the css. I also noticed that the horizontal line above the language bar disappeared when opaclanguagesdisplay pref was disabled. Could you have a look? Thanks, Marcel
Just two small extra remarks: 1) www.koha-community.org should be: koha-community.org 2) the <a> link should open in a new window; add target="_blank" attribute
I agree that the CSS should be handled in opac.css. The <span> already has an id, you can use that. Doing this will eliminate the other error I see, an all-caps STYLE attribute. It has to be lower-case in order to be valid XHTML. I disagree that the link should open a new window. If we really want to get technical about it, we could wrap it in a check of OPACURLOpenInNewWindow. Otherwise I say leave it.
(In reply to comment #21) > Owen, > I added you to the CC list of this report. > Patch looks good to me, except from the CSS related issues on the Powered by > Koha footer. I guess it would be better to move the hardcoded style stuff to > the css. yeah, i agree - i'll resumbit with that fix I also noticed that the horizontal line above the language bar > disappeared when opaclanguagesdisplay pref was disabled. yep, this was a deliberate 'feature' :) i'll removed it, as it introduces some unnecessary duplication to the template file
Created attachment 5099 [details] [review] patch
(In reply to comment #22) > Just two small extra remarks: > 1) www.koha-community.org should be: koha-community.org i agree :) fixed in my latest patch fyi, it looks like the canonical url for kc.org has ?recently? changed from www.koha-community.org to koha-community.org (which is a good thing, to me)
(In reply to comment #23) > I agree that the CSS should be handled in opac.css. yep, this is now done > Doing this will eliminate the other error I see, an > all-caps STYLE attribute. It has to be lower-case in order to be valid XHTML. fyi: the new patch tested ok as 'XHTML 1.0 Transitional' at http://validator.w3.org
Attempted to sign off, however can't convince it to apply :(
Created attachment 5402 [details] [review] patch
(In reply to comment #29) > Created attachment 5402 [details] [review] > patch newly attached patch is rebased from today's master/HEAD commit c849f7a90d4eed248bd53a00b8021c07e856e022 Adding the 155th developer to the history
> newly attached patch is rebased from today's master/HEAD ... and emailed to kc.org patches list ;)
Created attachment 5411 [details] [review] Bug 6636 - add 'powered by Koha' text to OPAC footer Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
Comment on attachment 5402 [details] [review] patch this one is obsolete if i'm not wrong (the signed-off version is the other)
mason, sorry, does not apply anymore :(( Could you rebase & resubmit (keeping "signed-off") If you do it this night, i'll try to QA it tomorrow, to have it for 3.6, it's a small but very usefull ENH !
Created attachment 5754 [details] [review] Bug 6636 - add 'powered by Koha' text to OPAC footer Signed-off-by: Robin Sheat <robin@catalyst.net.nz>
It was just a conflict in the CSS, I've fixed it against current master.
(In reply to comment #36) > It was just a conflict in the CSS, I've fixed it against current master. yep... and a 'single line of whitespace' conflict against master, too....
Created attachment 5762 [details] [review] Bug 6636 - add 'powered by Koha' text to OPAC footer Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> QA control: added () inside 2 [% IF %]
QA comment: * () were missing in a [% IF %] * also does some un-announced changes from TAB to 4 spaces, that's OK Marking passed QA
Pushed, please test
(In reply to comment #40) > Pushed, please test tested OK - marking resolved ;)