Bug 32634 - Add 'page-section' to various pages
Summary: Add 'page-section' to various pages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 30952
Blocks: 31757
  Show dependency treegraph
 
Reported: 2023-01-13 18:26 UTC by Owen Leonard
Modified: 2023-12-28 20:42 UTC (History)
4 users (show)

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


Attachments
Bug 32634: Add 'page-section' to various pages (59.67 KB, patch)
2023-01-13 20:18 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32634: Add 'page-section' to various pages (59.91 KB, patch)
2023-01-17 14:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32634: (follow-up) Restructure page headings (2.22 KB, patch)
2023-01-18 13:14 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32634: Add 'page-section' to various pages (43.25 KB, patch)
2023-03-29 11:49 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 32634: (follow-up) Restructure page headings (2.33 KB, patch)
2023-03-29 11:49 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 32634: Add 'page-section' to various pages (43.31 KB, patch)
2023-04-09 11:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32634: (follow-up) Restructure page headings (2.38 KB, patch)
2023-04-09 11:28 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 Owen Leonard 2023-01-13 18:26:29 UTC
A forthcoming patch will fix several templates that need to have main content wrapped in a "page-section" container.

 - members/accountline-details.tt
 - patroncards/manage.tt
 - reports/dictionary.tt
 - virtualshelves/shelves.tt
Comment 1 Owen Leonard 2023-01-13 20:17:27 UTC
Also including:

- offline_circ/enqueue_koc.tt
- offline_circ/list.tt
Comment 2 Owen Leonard 2023-01-13 20:18:23 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2023-01-16 20:06:51 UTC
Sorry about being nit-picky with the headings, but hoping to gain some more consistency:

1) accountline-details: The h2 heading should be inside the page-section to match the other pages.
2) patroncards/manage.tt: same as above.
3) reports/dictionary.tt: same as above.
4) Unrelated to the patch, but: New list > h1 should be outside page-section.
Comment 4 Owen Leonard 2023-01-17 14:50:56 UTC Comment hidden (obsolete)
Comment 5 David Nind 2023-01-17 23:26:12 UTC
I've tested and happy to sign off, but Katrin's comments in comment #3 are not all addressed - still 1, 2 and 4 to address. Maybe these should be in a separate bug?

Here is the data I used for the .koc file - save this into a text file and name as appropriate (uses patrons and item barcodes that are valid when using KTD and the default sample data):

Version=1.0	Generator=kocQt4	GeneratorVersion=1.0
2023-01-16 09:41:57 262	issue	23529000035676	39999000011418
2023-01-17 09:41:57 13	issue	23529000035676	39999000000559
2023-01-18 09:41:57 146	issue	23529000035676	39999000006131
2023-01-19 09:41:57 191	issue	23529000035676	39999000008036
Comment 6 Owen Leonard 2023-01-18 13:11:33 UTC
(In reply to David Nind from comment #5)
> I've tested and happy to sign off, but Katrin's comments in comment #3 are
> not all addressed - still 1, 2 and 4 to address

(In reply to Katrin Fischer from comment #3)

> 1) accountline-details: The h2 heading should be inside the page-section to
> match the other pages.

I didn't change this at first because for me the h1 wasn't displaying at all. Rather than fixing the template logic that causes this I think the correct solution is to display a generic "Transaction details" h1 since the h2 shows the transaction type.

> 2) patroncards/manage.tt: same as above.

This page looks correct to me. Am I missing it?

> 4) Unrelated to the patch, but: New list > h1 should be outside page-section.

This should be a separate issue, but we're running into some situations where we don't yet have a plan. The h1 on that page is inside a <legend>, which by definition is inside the fieldset (styled as page-section). The legend was added to comply with accessibility guidelines. Perhaps the legend should be screen-reader only and the h1 moved outside it.
Comment 7 Owen Leonard 2023-01-18 13:14:52 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2023-01-18 14:34:29 UTC
I'll try to have another look! 

It IS difficult. We had the h1 outside, h2 inside, legend inside "understanding" when we QA'd the first patches and wrote it down in the styleguide draft. The goal was to have at least some consistency, so I was trying ot keep that up, but ultimately it's not written down officially yet.

There is also that every page should have an h1 heading. I was not aware of the fieldsets needing a legend as well. Some more for the styleguide...

Maybe we should have a separate "style" meeting sometime to at least agree on some basics?
Comment 9 Lucas Gass 2023-01-30 23:14:33 UTC
> Maybe we should have a separate "style" meeting sometime to at least agree
> on some basics?

If you haven't done this already. +1
Comment 10 Magnus Enger 2023-03-29 10:09:13 UTC
Is this ready to be signed off, or are there things that need to be discussed?
Comment 11 Owen Leonard 2023-03-29 10:34:01 UTC
Ready for testing!
Comment 12 Magnus Enger 2023-03-29 11:49:12 UTC
Created attachment 148904 [details] [review]
Bug 32634: Add 'page-section' to various pages

This patch adds a "page-section" container div around the main section
of various 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:

- Locate a patron who has one or more account transactions.
  - Go to the Accounting tab -> Transactions. Click "Details" on one of
      the lines.
    - The "Details of debit" and "History" sections should be wrapped in
      .page-section.
- Go to Tools -> Patron card creator -> Manage -> Batches
  - If there is not a list of batches you will need to create at least
    one to see the page-section container.
- Go to Reports -> View dictionary.
  - If necessary, add a definition.
  - The "Current terms" table should be wrapped in .page-section.
- Go to Lists and view the contents of a list with one or more titles on
  it. The table of titles should be wrapped in .page-section.
- Go to Circulation -> Upload offline circulation file (.koc)
  - Upload a .koc file and click the "Add to offline circulation queue."
  - On the confirmation page the "Your file was uploaded" message should
    be wrapped in .page-section.
  - Click "View pending offline circulation actions"
    - The list of pending actions should be wrapped in .page-section

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 13 Magnus Enger 2023-03-29 11:49:16 UTC
Created attachment 148905 [details] [review]
Bug 32634: (follow-up) Restructure page headings

An error in template logic is preventing the page's h1 from displaying.
Rather than fixing this I have changed the h1 to display a generic
"Transaction details" h1. The h2 shows the transaction type (credit or
debit).

The h2 is now inside the .page-section.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Checked all the places mentioned in the commit message, and they
all have .page-section now.
Comment 14 Magnus Enger 2023-03-29 11:51:23 UTC
There were some minor conflicts applying the patches, but they were all about whitespace and I think I was able to resolve them.
Comment 15 Katrin Fischer 2023-04-09 11:28:04 UTC
Created attachment 149352 [details] [review]
Bug 32634: Add 'page-section' to various pages

This patch adds a "page-section" container div around the main section
of various 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:

- Locate a patron who has one or more account transactions.
  - Go to the Accounting tab -> Transactions. Click "Details" on one of
      the lines.
    - The "Details of debit" and "History" sections should be wrapped in
      .page-section.
- Go to Tools -> Patron card creator -> Manage -> Batches
  - If there is not a list of batches you will need to create at least
    one to see the page-section container.
- Go to Reports -> View dictionary.
  - If necessary, add a definition.
  - The "Current terms" table should be wrapped in .page-section.
- Go to Lists and view the contents of a list with one or more titles on
  it. The table of titles should be wrapped in .page-section.
- Go to Circulation -> Upload offline circulation file (.koc)
  - Upload a .koc file and click the "Add to offline circulation queue."
  - On the confirmation page the "Your file was uploaded" message should
    be wrapped in .page-section.
  - Click "View pending offline circulation actions"
    - The list of pending actions should be wrapped in .page-section

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Katrin Fischer 2023-04-09 11:28:07 UTC
Created attachment 149353 [details] [review]
Bug 32634: (follow-up) Restructure page headings

An error in template logic is preventing the page's h1 from displaying.
Rather than fixing this I have changed the h1 to display a generic
"Transaction details" h1. The h2 shows the transaction type (credit or
debit).

The h2 is now inside the .page-section.

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Checked all the places mentioned in the commit message, and they
all have .page-section now.
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Tomás Cohen Arazi 2023-04-10 10:38:21 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 18 Matt Blenkinsop 2023-04-18 12:33:24 UTC
Nice work everyone!

Pushed to stable for 22.11.x