Bug 32633 - Add 'page-section' to cataloging and authority pages
Summary: Add 'page-section' to cataloging and authority pages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Owen Leonard
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 30952
Blocks: 31757
  Show dependency treegraph
 
Reported: 2023-01-13 17:50 UTC by Owen Leonard
Modified: 2023-12-28 20:43 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03


Attachments
Bug 32633: Add 'page-section' to cataloging and authority pages (13.92 KB, patch)
2023-01-13 18:05 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32633: Add 'page-section' to cataloging and authority pages (13.97 KB, patch)
2023-01-16 19:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32633: Add 'page-section' to cataloging and authority pages (14.04 KB, patch)
2023-01-31 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32633: (QA follow-up) Move page-section nesting (24.90 KB, patch)
2023-01-31 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32633: (QA follow-up) Add handling for authorities merge (3.63 KB, patch)
2023-01-31 14:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2023-01-13 17:50:38 UTC
A few cataloging and authority templates need to have main content wrapped in a "page-section" container.

- includes/merge-record.inc
- authorities/searchresultlist.tt
- cataloguing/addbooks.tt
- cataloguing/additem.tt
Comment 1 Owen Leonard 2023-01-13 18:05:25 UTC
Created attachment 145302 [details] [review]
Bug 32633: Add 'page-section' to cataloging and authority pages

This patch adds a "page-section" container div around the main section
of cataloging and authority pages which lack it.

This patch contains indentation changes, so please ignore whitespace
when looking at the diff.

Apply the patch and view the following pages to confirm that the main
content is contained in a white box:

 - Cataloging -> Cataloging search results
 - Cataloging -> Edit items: The table of items and the item edit form
   should each be contained in a page-section.
 - Authorities -> Search results
 - Authorites -> Merge authorities: From the authorities search results
   list, click "Actions -> Merge" for one result, and then "Actions ->
   Merge" for another.
 - Click "Next" after selecting a merge reference.
   - The "Destination record" information should be contained in a
     page-section.
Comment 2 Lucas Gass 2023-01-13 20:48:28 UTC
Owen,

Should the page section on cataloging search include the pagination links? I know it is not a dataTable but I am comparing it to item search. 

I am still a bit confused on what the standard should be.
Comment 3 Katrin Fischer 2023-01-16 19:45:41 UTC
Created attachment 145340 [details] [review]
Bug 32633: Add 'page-section' to cataloging and authority pages

This patch adds a "page-section" container div around the main section
of cataloging and authority pages which lack it.

This patch contains indentation changes, so please ignore whitespace
when looking at the diff.

Apply the patch and view the following pages to confirm that the main
content is contained in a white box:

 - Cataloging -> Cataloging search results
 - Cataloging -> Edit items: The table of items and the item edit form
   should each be contained in a page-section.
 - Authorities -> Search results
 - Authorites -> Merge authorities: From the authorities search results
   list, click "Actions -> Merge" for one result, and then "Actions ->
   Merge" for another.
 - Click "Next" after selecting a merge reference.
   - The "Destination record" information should be contained in a
     page-section.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Katrin Fischer 2023-01-30 22:57:19 UTC
Just realized.. I SO here. Can someone else take it for QA please?
Comment 5 Martin Renvoize 2023-01-31 14:19:04 UTC
Created attachment 145872 [details] [review]
Bug 32633: Add 'page-section' to cataloging and authority pages

This patch adds a "page-section" container div around the main section
of cataloging and authority pages which lack it.

This patch contains indentation changes, so please ignore whitespace
when looking at the diff.

Apply the patch and view the following pages to confirm that the main
content is contained in a white box:

 - Cataloging -> Cataloging search results
 - Cataloging -> Edit items: The table of items and the item edit form
   should each be contained in a page-section.
 - Authorities -> Search results
 - Authorites -> Merge authorities: From the authorities search results
   list, click "Actions -> Merge" for one result, and then "Actions ->
   Merge" for another.
 - Click "Next" after selecting a merge reference.
   - The "Destination record" information should be contained in a
     page-section.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2023-01-31 14:19:09 UTC
Created attachment 145873 [details] [review]
Bug 32633: (QA follow-up) Move page-section nesting

This follow-up makes a small change to the page-section nesting on the
Cataloging and Authorities search results pages.  We move the 'h2'
outside of the page section but include the paging and number of results
inside with the results tables themselves.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2023-01-31 14:19:14 UTC
Created attachment 145874 [details] [review]
Bug 32633: (QA follow-up) Add handling for authorities merge

When you first click 'actions -> merge' from a result in the authories
search page you will see a new 'Merging with authority: ' text appear at
the top of the search results.

Prior to this patch, the text appears outside of the white page-section
and is almost un-noticable.

After this patch, we add a page-section with bg-info (blue backround)
around the merge message which makes it stand up more clearly.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2023-01-31 14:20:44 UTC
I agreed with Lucas on the pageing so did that as a QA follow-up.

It also highlighted to me that the merge message wasn't entirely clear sitting outside the page-section but without any background of it's own.. So I added a further QA follow-up to clarify that too.

No regressions found and code all passes QA tests...

Passing QA
Comment 9 Tomás Cohen Arazi 2023-02-02 15:02:11 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 10 Jacob O'Mara 2023-02-06 15:58:29 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 11 Lucas Gass 2023-02-14 21:29:29 UTC
Missing dependencies for 22.05.x, no backport.