|
Description
Caroline Cyr La Rose
2023-07-25 22:04:15 UTC
Created attachment 155233 [details] [review] Bug 34412: Inconsistencies in System preferences page titles, breadcrumbs, and header This patch makes changes to the markup of various areas of the local use system preferences template in order to make it more consistent with other pages in Koha. To test, apply the patch and go to Administration -> System preferences -> Local use. View each variation of the page to confirm that breadcrumb navigation, page title, and page headings are consistent with each other. - On the main page confirm that the heading is "Local use system preferences" - New preference: - The form on this page has been updated to make the markup structure more consistent with other pages, with the heading outside the white box of the form. - The two portions of the form have been combined into one so that the layout of the form is more consistent with similar forms. - Edit preference - Delete: - The deletion confirmation page has been reformatted to use an "alert" style dialog with styled confirmation buttons. - The "Data deleted" view has been reformatted to use a "message" style dialog. Comment on attachment 155233 [details] [review] Bug 34412: Inconsistencies in System preferences page titles, breadcrumbs, and header Review of attachment 155233 [details] [review]: ----------------------------------------------------------------- I have a few comments but none of them are blockers imo ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/systempreferences.tt @@ +13,4 @@ > [% END %] › > [% END %] > [% IF ( add_validate ) %] > + [% t("Preference added") | html %] › I couldn't find a way to see this text but I'm not too worried about it. @@ +91,5 @@ > > [% IF ( add_form ) %] > + <h1> > + [% IF ( modify ) %] > + Modify a local use system preference I think this could probably be "Modify preference '[% searchfield | html %]'", but not a deal breaker. @@ +283,5 @@ > [% END %] > > [% IF ( delete_confirm ) %] > + <div class="dialog alert"> > + <h1>Confirm deletion of [% searchfield | html %]?</h1> It would be a good idea to wrap [% searchfield | html %] in single quotes, but since this is the same before and after the change I don't think it's in scope. Created attachment 155250 [details] [review] Bug 34412: Inconsistencies in System preferences page titles, breadcrumbs, and header This patch makes changes to the markup of various areas of the local use system preferences template in order to make it more consistent with other pages in Koha. To test, apply the patch and go to Administration -> System preferences -> Local use. View each variation of the page to confirm that breadcrumb navigation, page title, and page headings are consistent with each other. - On the main page confirm that the heading is "Local use system preferences" - New preference: - The form on this page has been updated to make the markup structure more consistent with other pages, with the heading outside the white box of the form. - The two portions of the form have been combined into one so that the layout of the form is more consistent with similar forms. - Edit preference - Delete: - The deletion confirmation page has been reformatted to use an "alert" style dialog with styled confirmation buttons. - The "Data deleted" view has been reformatted to use a "message" style dialog. Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 155262 [details] [review] Bug 34412: (follow-up) Clarify page heading, remove unused markup This patch updates the heading for the "modify" view so that it includes the name of the preference being modified. The patch also removes markup related to the "add_validate" state--a view confirming that a preference has been added--because that view doesn't exist. I feel it would be nicer to make some changes here:
* We agreed a while not to use "preference" or "sys pref" to always give the full context (consistency). So I think we should use "New system preference" etc.
* Instead of naming the placeholder searchfield, we should also name it {system_preference} because that is what will show up for translators and searchfield would likely cause confusion.
[% tx("Confirm deletion of preference '{searchfield}'", { searchfield = searchfield }) | html %] ›
Created attachment 155393 [details] [review] Bug 34412: (QA follow-up) Terminology updates - Use "system preference" instead of just "preference" - Use "system_preference" as placeholder in translatable strings This patch also updates a few places where the placeholder i18n syntax can be used for consistency. Created attachment 155401 [details] [review] Bug 34412: Inconsistencies in System preferences page titles, breadcrumbs, and header This patch makes changes to the markup of various areas of the local use system preferences template in order to make it more consistent with other pages in Koha. To test, apply the patch and go to Administration -> System preferences -> Local use. View each variation of the page to confirm that breadcrumb navigation, page title, and page headings are consistent with each other. - On the main page confirm that the heading is "Local use system preferences" - New preference: - The form on this page has been updated to make the markup structure more consistent with other pages, with the heading outside the white box of the form. - The two portions of the form have been combined into one so that the layout of the form is more consistent with similar forms. - Edit preference - Delete: - The deletion confirmation page has been reformatted to use an "alert" style dialog with styled confirmation buttons. - The "Data deleted" view has been reformatted to use a "message" style dialog. Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 155402 [details] [review] Bug 34412: (follow-up) Clarify page heading, remove unused markup This patch updates the heading for the "modify" view so that it includes the name of the preference being modified. The patch also removes markup related to the "add_validate" state--a view confirming that a preference has been added--because that view doesn't exist. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Created attachment 155403 [details] [review] Bug 34412: (QA follow-up) Terminology updates - Use "system preference" instead of just "preference" - Use "system_preference" as placeholder in translatable strings This patch also updates a few places where the placeholder i18n syntax can be used for consistency. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de> Pushed to master for 23.11. Nice work everyone, thanks! Like others, not backported to 23.05.x |