Bug 28927 - Id opacmainuserblock used twice in OPAC
Summary: Id opacmainuserblock used twice in OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-08-31 06:51 UTC by Fridolin Somers
Modified: 2022-06-06 20:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch removes redundant div with id 'opacmainuserblock' and 'opacheader' since there is already this id generated by HTML customization.
Version(s) released in:
21.11.00,21.05.05,20.11.11


Attachments
Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock". (3.13 KB, patch)
2021-08-31 07:12 UTC, Fridolin Somers
Details | Diff | Splinter Review
Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock". (3.20 KB, patch)
2021-09-01 09:06 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 28927: Fix id opacmainuserblock used twice in OPAC (3.25 KB, patch)
2021-09-08 08:56 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS (929 bytes, patch)
2021-09-08 09:07 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 28927: Fix id opacmainuserblock used twice in OPAC (3.46 KB, patch)
2021-09-11 11:05 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS (1.05 KB, patch)
2021-09-11 11:05 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28927: Fix id opacmainuserblock used twice in OPAC (2.53 KB, patch)
2021-09-22 06:27 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS (1.05 KB, patch)
2021-09-22 06:27 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 28927: Fix id opacmainuserblock used twice in OPAC (2.58 KB, patch)
2021-09-22 14:56 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS (1.10 KB, patch)
2021-09-22 14:56 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28927: Fix id opacmainuserblock used twice in OPAC (2.59 KB, patch)
2021-09-26 13:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS (1.10 KB, patch)
2021-09-26 13:30 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 Fridolin Somers 2021-08-31 06:51:58 UTC
Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".

I think template block "koha_news_block" should add a class instead of an id.
It fixes the bug and also it may be used several times in same page in the future.
Comment 1 Fridolin Somers 2021-08-31 07:12:03 UTC
Created attachment 124267 [details] [review]
Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".

I think template block "koha_news_block" should add a class (lowercase) instead of an id.
It fixes the bug and also it may be used several times in same page in the future.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see class="opacmainuserblock" and not id="OpacMainUserBlock"
Comment 2 Biblibre Sandboxes 2021-09-01 09:06:38 UTC
Created attachment 124318 [details] [review]
Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".

I think template block "koha_news_block" should add a class (lowercase) instead of an id.
It fixes the bug and also it may be used several times in same page in the future.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see class="opacmainuserblock" and not id="OpacMainUserBlock"

https://bugs.koha-community.org/show_bug.cgi?id=28927

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 3 Sally 2021-09-01 09:06:54 UTC
Works as described.
Comment 4 Joonas Kylmälä 2021-09-05 10:21:56 UTC
Thanks, I think this change looks reasonable, though some things to fix.

1) Could you fix up the commit message, you mention that this fixes a bug but don't say which one, also mention in the commit message what the change does and use the format "Bug XXXX: Change X thing". I'm guessing you are doing this change to simplify (custom) CSS rules?

2) It seems to break the OpacHeader / opacheader CSS rule defined in the line

koha/koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss:#opacheader {
Comment 5 Fridolin Somers 2021-09-08 08:54:25 UTC
(In reply to Joonas Kylmälä from comment #4)
> Thanks, I think this change looks reasonable, though some things to fix.
> 
> 1) Could you fix up the commit message, you mention that this fixes a bug
> but don't say which one, also mention in the commit message what the change
> does and use the format "Bug XXXX: Change X thing". I'm guessing you are
> doing this change to simplify (custom) CSS rules?

Oups sorry i did a git mistake.

> 
> 2) It seems to break the OpacHeader / opacheader CSS rule defined in the line
> 
> koha/koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss:#opacheader {

Good point i'm on it
Comment 6 Fridolin Somers 2021-09-08 08:56:29 UTC
Created attachment 124637 [details] [review]
Bug 28927: Fix id opacmainuserblock used twice in OPAC

Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".

I think template block "koha_news_block" should add a class (lowercase) instead of an id.
It fixes the bug and also it may be used several times in same page in the future.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see class="opacmainuserblock" and not id="OpacMainUserBlock"

https://bugs.koha-community.org/show_bug.cgi?id=28927

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 7 Fridolin Somers 2021-09-08 09:04:32 UTC
(In reply to Joonas Kylmälä from comment #4)
> 
> 2) It seems to break the OpacHeader / opacheader CSS rule defined in the line
> 
> koha/koha-tmpl/opac-tmpl/bootstrap/css/src/opac.scss:#opacheader {

Actually this rule does nothing, it defines same background color as "body" :
#opacheader {
    background-color: #fcf9fc;
}

I propose de remove it.
Comment 8 Fridolin Somers 2021-09-08 09:07:20 UTC
Created attachment 124638 [details] [review]
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS

Actually this rule does nothing, it defines same background color as "body" : #fcf9fc

Test plan :
1) Without patch look at OPAC header
2) With patch OPAC header should have same background color
Comment 9 Joonas Kylmälä 2021-09-11 11:05:06 UTC
Created attachment 124793 [details] [review]
Bug 28927: Fix id opacmainuserblock used twice in OPAC

Since preference OpacMainUserBlock is an HTML customization, there is an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".

I think template block "koha_news_block" should add a class (lowercase) instead of an id.
It fixes the bug and also it may be used several times in same page in the future.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see class="opacmainuserblock" and not id="OpacMainUserBlock"

https://bugs.koha-community.org/show_bug.cgi?id=28927

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Rebased-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 10 Joonas Kylmälä 2021-09-11 11:05:09 UTC
Created attachment 124794 [details] [review]
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS

Actually this rule does nothing, it defines same background color as "body" : #fcf9fc

Test plan :
1) Without patch look at OPAC header
2) With patch OPAC header should have same background color

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 11 Joonas Kylmälä 2021-09-11 11:05:45 UTC
Passing QA as Sally already signed this off.
Comment 12 Jonathan Druart 2021-09-20 10:45:10 UTC
Current behaviour is definitely wrong, but I am wondering why we don't keep the id with the same capitalization (not lowered).
Comment 13 Jonathan Druart 2021-09-20 11:23:15 UTC
Asked Owen on IRC:

oleonard> Hm, no... There should be a single OpacMainUserBlock, not two. The class would be correct if it were repeated for each additional content entry, but it's not.
Joubu> yes, that's my point
oleonard> I agree with the change removing the divs from opac-main.tt and opac-registration-confirmation.tt, those are cleanups I should have done earlier.
oleonard> However I think the id in html_helpers.inc should stay.
Comment 14 Fridolin Somers 2021-09-22 06:27:03 UTC
Created attachment 125125 [details] [review]
Bug 28927: Fix id opacmainuserblock used twice in OPAC

Since preference OpacMainUserBlock is now an HTML customization,
there is actually an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".
Same for OPACNavRight in opac-registration-confirmation.tt

This patch removed id from page to keep the one comming from HTML customization include.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see id="OpacMainUserBlock" once and not id="opacmainuserblock"
5) Same for id="opacnavright" in opac-registration-confirmation.tt
Comment 15 Fridolin Somers 2021-09-22 06:27:52 UTC
Created attachment 125126 [details] [review]
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS

Actually this rule does nothing, it defines same background color as "body" : #fcf9fc

Test plan :
1) Without patch look at OPAC header
2) With patch OPAC header should have same background color

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 16 Fridolin Somers 2021-09-22 06:31:33 UTC
(In reply to Jonathan Druart from comment #13)
> Asked Owen on IRC:
> 
> oleonard> Hm, no... There should be a single OpacMainUserBlock, not two. The
> class would be correct if it were repeated for each additional content
> entry, but it's not.
> Joubu> yes, that's my point
> oleonard> I agree with the change removing the divs from opac-main.tt and
> opac-registration-confirmation.tt, those are cleanups I should have done
> earlier.
> oleonard> However I think the id in html_helpers.inc should stay.

Ok I'm in.

Main patch needs a new signoff.

I've kept followup patch if its OK.
Comment 17 Owen Leonard 2021-09-22 14:56:48 UTC
Created attachment 125137 [details] [review]
Bug 28927: Fix id opacmainuserblock used twice in OPAC

Since preference OpacMainUserBlock is now an HTML customization,
there is actually an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".
Same for OPACNavRight in opac-registration-confirmation.tt

This patch removed id from page to keep the one comming from HTML customization include.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see id="OpacMainUserBlock" once and not id="opacmainuserblock"
5) Same for id="opacnavright" in opac-registration-confirmation.tt

https://bugs.koha-community.org/show_bug.cgi?id=28927

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Owen Leonard 2021-09-22 14:56:52 UTC
Created attachment 125138 [details] [review]
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS

Actually this rule does nothing, it defines same background color as "body" : #fcf9fc

Test plan :
1) Without patch look at OPAC header
2) With patch OPAC header should have same background color

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Katrin Fischer 2021-09-26 13:30:21 UTC
Created attachment 125332 [details] [review]
Bug 28927: Fix id opacmainuserblock used twice in OPAC

Since preference OpacMainUserBlock is now an HTML customization,
there is actually an HTML tags with id="opacmainuserblock" and one with id="OpacMainUserBlock".
Same for OPACNavRight in opac-registration-confirmation.tt

This patch removed id from page to keep the one comming from HTML customization include.

Test plan :
1) Create an HTML customization for OpacMainUserBlock with some text
2) Go to OPAC main page
3) Check HTML code of the page
4) Check you see id="OpacMainUserBlock" once and not id="opacmainuserblock"
5) Same for id="opacnavright" in opac-registration-confirmation.tt

https://bugs.koha-community.org/show_bug.cgi?id=28927

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 20 Katrin Fischer 2021-09-26 13:30:26 UTC
Created attachment 125333 [details] [review]
Bug 28927: (follow-up) remove rule on id=opacheader in opac CSS

Actually this rule does nothing, it defines same background color as "body" : #fcf9fc

Test plan :
1) Without patch look at OPAC header
2) With patch OPAC header should have same background color

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 21 Jonathan Druart 2021-09-28 13:38:41 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 22 Kyle M Hall 2021-10-08 11:00:36 UTC
Pushed to 21.05.x for 21.05.05
Comment 23 Fridolin Somers 2021-10-09 02:26:49 UTC
Pushed to 20.11.x for 20.11.11
Comment 24 Victor Grousset/tuxayo 2021-10-17 01:15:49 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.