Bug 32634

Also including:

- offline_circ/enqueue_koc.tt
- offline_circ/list.tt

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.

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

(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.

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?

> Maybe we should have a separate "style" meeting sometime to at least agree
> on some basics?

If you haven't done this already. +1

Is this ready to be signed off, or are there things that need to be discussed?

Ready for testing!

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>

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.

There were some minor conflicts applying the patches, but they were all about whitespace and I think I was able to resolve them.

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>

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>

Pushed to master for 23.05.

Nice work everyone, thanks!

Nice work everyone!

Pushed to stable for 22.11.x