There aren't distinct html classes on the various sections and buttons on /cgi-bin/koha/circ/circulation-home.pl and it makes things hard to target with CSS.
Created attachment 177319 [details] [review] Bug 38994: Add classes to circulation module homepage To test: 1: inspect /koha/circ/circulation-home.pl, see most elements lack distinct classes 2: apply patch, reload page 3: inspect page again, confirm nothing has changed in display 4: confirm page elements have distinct classes now and can be accurately targetted with CSS
Applying patches produces following error: Applying: Bug 38994: Add classes to circulation module homepage Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation-home.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation-home.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation-home.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 38994: Add classes to circulation module homepage hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-38994-Add-classes-to-circulation-module-homepa-xIUMjF.patch
Created attachment 180782 [details] [review] Bug 38994: Add classes to circulation module homepage To test: 1: inspect /koha/circ/circulation-home.pl, see most elements lack distinct classes 2: apply patch, reload page 3: inspect page again, confirm nothing has changed in display 4: confirm page elements have distinct classes now and can be accurately targetted with CSS
Created attachment 180795 [details] [review] Bug 38994: Add classes to circulation module homepage To test: 1: inspect /koha/circ/circulation-home.pl, see most elements lack distinct classes 2: apply patch, reload page 3: inspect page again, confirm nothing has changed in display 4: confirm page elements have distinct classes now and can be accurately targetted with CSS Signed-off-by: Jason Robb <jrobb@sekls.org>
I reckon for "Offline circulation", it would be good to have a class for the whole section. For instance, for bug 39378, if someone wanted to hide this with CSS, they'd need to do it for the whole section.
(In reply to David Cook from comment #5) > I reckon for "Offline circulation", it would be good to have a class for the > whole section. > > For instance, for bug 39378, if someone wanted to hide this with CSS, they'd > need to do it for the whole section. That section already has an id, so this works: #offline-circulation { display: none; }
(In reply to Andrew Fuerste-Henry from comment #6) > That section already has an id, so this works: > #offline-circulation { > display: none; > } Ah, nice one! Looks like I misread the patch! Thanks for the correction :).
Created attachment 180894 [details] [review] Bug 38994: Add classes to circulation module homepage To test: 1: inspect /koha/circ/circulation-home.pl, see most elements lack distinct classes 2: apply patch, reload page 3: inspect page again, confirm nothing has changed in display 4: confirm page elements have distinct classes now and can be accurately targetted with CSS Signed-off-by: Jason Robb <jrobb@sekls.org> Signed-off-by: David Cook <dcook@prosentient.com.au>
I really like this. The more hooks on HTML elements the better imho. Technically, some of these classes probably could've/should've been IDs, but I don't think it matters, especially since you can chain them together with the body ID of "circ_circulation-home". So thumbs up from me.
I agree about the id vs. class predicament, but I don't have a strong argument for changing it. Some of the classes are quite general, but there doesn't appear to be a conflict as far as I can tell. Not blocker.
Pushed for 25.05! Well done everyone, thank you!