Created attachment 162715 [details] [review] Bug 36221: Preparation This refactors the code for more DRY
Created attachment 162716 [details] [review] Bug 36221: OPAC Styling Test plan, dont apply patches yet: 1) Enable ILLModule 2) Create a new OPAC ILL request, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?method=create&backend=Standard 3) Notice the style. Apply patches. Repeat
Created attachment 162933 [details] [review] Bug 36221: OPAC Styling Test plan, dont apply patches yet: 1) Enable ILLModule 2) Create a new OPAC ILL request, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?method=create&backend=Standard 3) Notice the style. Apply patches. Repeat Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 162934 [details] [review] Attachment to Bug 36221 - Improve styling of Standard backend create OPAC form Bug 36221: Preparation This refactors the code for more DRY Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 162948 [details] [review] Bug 36221: Preparation This refactors the code for more DRY Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 162949 [details] [review] Bug 36221: OPAC Styling Test plan, dont apply patches yet: 1) Enable ILLModule 2) Create a new OPAC ILL request, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?method=create&backend=Standard 3) Notice the style. Apply patches. Repeat Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Patches became out of order after Lisette's sign-off and did not apply. It's now fixed.
Created attachment 162970 [details] [review] Bug 36221: Move patron section to top of create form
Created attachment 162971 [details] [review] Bug 36221: Update 'Create' button class btn-primary should be used here, not btn-default
Thank you for looking and sign-off, Lisette! However I've revisited and added 2 additional patches in case you're available to take a look at those too! Cheers!
Created attachment 165462 [details] [review] Bug 36221: Preparation This refactors the code for more DRY Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 165463 [details] [review] Bug 36221: OPAC Styling Test plan, dont apply patches yet: 1) Enable ILLModule 2) Create a new OPAC ILL request, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?method=create&backend=Standard 3) Notice the style. Apply patches. Repeat Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Created attachment 165464 [details] [review] Bug 36221: Move patron section to top of create form
Created attachment 165465 [details] [review] Bug 36221: Update 'Create' button class btn-primary should be used here, not btn-default
Comment on attachment 165462 [details] [review] Bug 36221: Preparation Review of attachment 165462 [details] [review]: ----------------------------------------------------------------- ::: Koha/ILL/Backend/shared-includes/form_input_helpers.inc @@ +30,5 @@ > +[% BLOCK journal_text_input_fields %] > + [% PROCESS ill_text_input_field id = 'title' label = t('Title') value = whole.value.other.title %] > + [% PROCESS ill_text_input_field id = 'volume' label = t('Volume') value = whole.value.other.volume %] > + [% PROCESS ill_text_input_field id = 'issue' label = t('Issue number') value = whole.value.other.issue %] > + [% PROCESS ill_text_input_field id = 'year' label = t('Year') value = whole.value.other.issue %] Should be whole.value.other.year
Very nice improvements :) Nothing wrong found when testing. I didn't finish proofreading but there is at least the above
Created attachment 173706 [details] [review] Bug 36221: (QA follow-up): Fix year value
Created attachment 173752 [details] [review] Bug 36221: Preparation This refactors the code for more DRY Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173753 [details] [review] Bug 36221: OPAC Styling Test plan, dont apply patches yet: 1) Enable ILLModule 2) Create a new OPAC ILL request, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?method=create&backend=Standard 3) Notice the style. Apply patches. Repeat Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173754 [details] [review] Bug 36221: Move patron section to top of create form Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173755 [details] [review] Bug 36221: Update 'Create' button class btn-primary should be used here, not btn-default Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173756 [details] [review] Bug 36221: (QA follow-up): Fix year value Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Just another signoff.. hoping Victor finished QA today
Somehow when picking the type ("Create new request" => "Type*") the type ends up as the type attr of each <input> :o <input class="form-control input-fluid" type="article" name="title" id="title" value=""> It happens for all values of the <select> except DVD. Whose field have the old style. Because dvd.inc wasn't touched in these patches. So there is also that.
> <input class="form-control input-fluid" type="article" [...]> Happens on staff interface also. The origin is form_input_helpers.inc => BLOCK ill_text_input_field
Comment on attachment 173753 [details] [review] Bug 36221: OPAC Styling Review of attachment 173753 [details] [review]: ----------------------------------------------------------------- ::: Koha/ILL/Backend/shared-includes/custom_fields.inc @@ +7,5 @@ > [% FOREACH key IN keys %] > <li class="form-horizontal"> > + [% IF opac %] > + <input type="text" placeholder="key" class="form-control input-fluid custom-field-input custom-name" name="custom_key" value="[% key | html %]"> > + <input type="text" placeholder="value" class="form-control input-fluid custom-field-input" name="custom_value" id="custom-value" value="[% values.$i | html %]"> After checking. The two placeholders are not translatable. ::: Koha/ILL/Backend/shared-includes/shared.js @@ +7,5 @@ > + '<input type="text" class="custom-name ' + > + ( opac ? 'form-control input-fluid custom-field-input' : '') + > + '" name="custom_key" placeholder="key">' + > + '<input type="text" id="custom-value" name="custom_value" class="' + > + ( opac ? 'form-control input-fluid custom-field-input' : '') +'" placeholder="value"> ' + These placeholders certainly aren't translatable either.
It seems placeholder values should have been extracted as translatable strings directly. Oh, it's not even about placeholders. When changing the language, the Create and Cancel button are still untranslated. That might be that having .inc files in Koha/ILL/Backend can't work. The translation deployment script don't even have a place to copy the .inc files and translate them. But it's a problem that made through some previous ticket and not this one. So here there is no problem about the placeholders in the .inc files. (is there a ticket about this? didn't found one but I'm not sure about my search) There is still a problem about those in shared-includes/shared.js Solution: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS2:_Enabling_translation_of_language_strings_in_embedded_JavaScript https://wiki.koha-community.org/wiki/Coding_Guidelines#JS5:_Enabling_translation_of_language_strings_in_linked_JavaScript
> (is there a ticket about this? didn't found one but I'm not sure about my search) Thanks Pedro for opening Bug 38340 :)
Created attachment 173939 [details] [review] Bug 36221: (QA follow-up): Fix incorrect input type on HTML
Created attachment 173940 [details] [review] Bug 36221: (QA follow-up): Apply styling improvements to DVD qa tools throw: ill_text_input_field: not found But I believe this is a false positive. We can clearly see it working by going to: http://localhost:8080/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard And picking 'DVD' or any other type.
Created attachment 173941 [details] [review] Bug 36221: (QA follow-up): Fix translatability in JS concat
Thanks for the follow-ups :) Almost everything looks good. Remains some OPAC styling that ended up in the staff interface: https://pic.infini.fr/jf8dBvZK/BhOdvYNu.png It's when making a request with custom fields, then editing it's item metadata and adding fields. The new ones are added with the OPAC style. In the screenshot, I emptied the fields that were already populated (FOREACH key IN keys) to best compare them with the ones added by the JS. Bad news, this is true on the staff page: ( opac ? 'form-control input-fluid custom-field-input' : '') + console.log(opac) => Array [] ----- Testing notes, to have custom field pre populated in the OPAC, use a request like this (thanks Pedro): http://localhost:8080/cgi-bin/koha/opac-illrequests.pl?atitle=Music%20in%20an%20Emergent%20History%20of%20Psychology.&aulast=Klempe%20SH&backend=Standard&cardnumber=kohaadmin&custom_field=example1&date=2024&doi=10.1007%2Fs12124-023-09778-9&genre=article&issn=19363567&issue=1&custom_field2=example2&method=create&opac=1&openurl=1&pages=1-11&spage=1&title=Integrative%20psychological%20%26%20behavioral%20science&volume=58
Created attachment 173965 [details] [review] Bug 36221: (QA follow-up): add placeholder attr to staff custom fields In the prepopulated fields when editing item metadata that had custom fields (tt generated). So it's consistent when adding new empty fields (done in JS)
> In the screenshot, I emptied the fields that were > already populated (FOREACH key IN keys) to best > compare them with the ones added by the JS. Which showed the edited fields having no placeholder and the new empty ones having one. Above patch fixed this.
Created attachment 173969 [details] [review] Bug 36221: (QA follow-up): Fix opac check To address Victor's comment #32
Created attachment 174017 [details] [review] Bug 36221: Preparation This refactors the code for more DRY Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174018 [details] [review] Bug 36221: Move patron section to top of create form Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174019 [details] [review] Bug 36221: (QA follow-up): Apply styling improvements to DVD qa tools throw: ill_text_input_field: not found But I believe this is a false positive. We can clearly see it working by going to: http://localhost:8080/cgi-bin/koha/opac-illrequests.pl?op=add_form&backend=Standard And picking 'DVD' or any other type. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174020 [details] [review] Bug 36221: OPAC Styling Test plan, dont apply patches yet: 1) Enable ILLModule 2) Create a new OPAC ILL request, visit: <opac_url>/cgi-bin/koha/opac-illrequests.pl?method=create&backend=Standard 3) Notice the style. Apply patches. Repeat Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174021 [details] [review] Bug 36221: Update 'Create' button class btn-primary should be used here, not btn-default Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174022 [details] [review] Bug 36221: (QA follow-up): Fix year value Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174023 [details] [review] Bug 36221: (QA follow-up): Fix incorrect input type on HTML Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174024 [details] [review] Bug 36221: (QA follow-up): Fix translatability in JS concat Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174025 [details] [review] Bug 36221: (QA follow-up): add placeholder attr to staff custom fields In the prepopulated fields when editing item metadata that had custom fields (tt generated). So it's consistent when adding new empty fields (done in JS) Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174026 [details] [review] Bug 36221: (QA follow-up): Fix opac check To address Victor's comment #32 Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 174027 [details] [review] Bug 36221: (QA follow-up): fix space between the 2 custom fields <input> It was missing in the JS that adds new empty custom fields. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Works, makes sense, QA script happy, code looks good, passing QA :)
Please check: FAIL Koha/ILL/Backend/intra-includes/create.inc FAIL valid_template /shared-includes/form_input_helpers.inc: not found FAIL Koha/ILL/Backend/intra-includes/edititem.inc FAIL valid_template /shared-includes/form_input_helpers.inc: not found FAIL Koha/ILL/Backend/opac-includes/create.inc FAIL valid_template /shared-includes/form_input_helpers.inc: not found OK Koha/ILL/Backend/shared-includes/custom_fields.inc OK Koha/ILL/Backend/shared-includes/form_input_helpers.inc FAIL Koha/ILL/Backend/shared-includes/forms/article.inc FAIL valid_template journal_text_input_fields: not found FAIL Koha/ILL/Backend/shared-includes/forms/book.inc FAIL valid_template book_text_input_fields: not found FAIL Koha/ILL/Backend/shared-includes/forms/chapter.inc FAIL valid_template book_text_input_fields: not found FAIL Koha/ILL/Backend/shared-includes/forms/conference.inc FAIL valid_template ill_text_input_field: not found FAIL Koha/ILL/Backend/shared-includes/forms/dvd.inc FAIL valid_template ill_text_input_field: not found FAIL Koha/ILL/Backend/shared-includes/forms/journal.inc FAIL valid_template journal_text_input_fields: not found FAIL Koha/ILL/Backend/shared-includes/forms/resource.inc FAIL valid_template ill_text_input_field: not found FAIL Koha/ILL/Backend/shared-includes/forms/thesis.inc FAIL valid_template ill_text_input_field: not found
(In reply to Katrin Fischer from comment #48) > Please check: 1. article.inc is processing a block called journal_text_input_fields 2. article.inc is dynamically invoked by create.inc 3. create.inc processes the form_input_helpers.inc file 4. form_input_helpers.inc file contains the journal_text_input_fields block required by 1. The QA script expects the block to be defined where it's invoked (in this case article.inc). But it's not. This is a false positive as far as I'm concerned though happy to hear others opinions.
(In reply to Victor Grousset/tuxayo from comment #47) > Works, makes sense, QA script happy, code looks good, passing QA :) QA script was not happy for me - could it be a change from another bug changing things?
I've called in some reinforcements on this one. I'd like to make sure the QA test fails are really false positives as noone seems to have noted them before. There are also some new inconsistencies introduced here that we should take a look at.
This has some interface issues that require follow-ups: - We don't use an asterisk to denote required fields. In the OPAC you should add a hint after the form field: <div class="required_label required">Required</div> - The dropdown menus don't look right. There's no dropdown indicator (a down arrow). It wasn't immediately obvious to me why not! - Can we add client-side form validation? Or at least 'required="required"' to trigger the browser's built-in validation? - Error messages should be styled like error messages, using a div with the classes "alert alert-warning" - Capitalization rules should be followed, e.g. "Please Note" should be "Please note" - Do we need both the heading "New interlibrary loan request" and the heading "Create a manual ILL request"?
Keeping the keyword here as this mostly concerns GUI, but we need to act fast please.
(In reply to Owen Leonard from comment #52) > - The dropdown menus don't look right. There's no dropdown indicator (a down > arrow). It wasn't immediately obvious to me why not! The <select>s need the "form-select" class!
Is it correct that I can submit a request without any fields like title, author, etc. filled in?
Created attachment 174385 [details] [review] Bug 36221: (follow-up) Style and markup fixes This patch makes several changes to make the updated forms more consistent with how other pages work: - Form labels should have ":" after them. - Form fields should have a hint when they are required. - Required fields should not be indicated with an asterisk. - In the OPAC, <select> should have a "form-select" class if the form is otherwise being styled with Bootstrap classes. - Primary submit buttons should have a "btn btn-primary" class. - The Font Awesome 6 icon for a delete operation is "fa-trash-can." - Error messages should be wrapped in <div class="alert alert-warning">. - All language strings in JS files should be wrapped in __(''). - Terminology: "Library" instead of "Branch," "Patron" instead of "Member."
Should we be using [% PROCESS options_for_libraries %] anywhere there is a library dropdown?
Created attachment 174398 [details] [review] Bug 36221: (follow-up) Style and markup fixes This patch makes several changes to make the updated forms more consistent with how other pages work: - Form labels should have ":" after them. - Form fields should have a hint when they are required. - Required fields should not be indicated with an asterisk. - In the OPAC, <select> should have a "form-select" class if the form is otherwise being styled with Bootstrap classes. - Primary submit buttons should have a "btn btn-primary" class. - The Font Awesome 6 icon for a delete operation is "fa-trash-can." - Error messages should be wrapped in <div class="alert alert-warning">. - All language strings in JS files should be wrapped in __(''). - Terminology: "Library" instead of "Branch," "Patron" instead of "Member." Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Hi guys thanks for your time and input here. I've submitted patches to 35570 (as requested by Katrin, not yet pushed) that contain fixes to some of the issues being raised here (terminology, translation, etc): https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=35570&attachment=173916 https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=35570&attachment=173917 https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=35570&attachment=173918 Up to RM what to pick and push but it's a bit of a shame we're fixing the same things in different places and overlapping in time investment. If my follow-ups on bug 35570 have gone unnoticed then please let me know what I should've done differently. I'm on board with everything Owen is suggesting, and added my sign-off to his patch. Please let me know what else is required of me to help move this.
(In reply to Pedro Amorim from comment #59) > Hi guys thanks for your time and input here. > > I've submitted patches to 35570 (as requested by Katrin, not yet pushed) > that contain fixes to some of the issues being raised here (terminology, > translation, etc): > https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter. > html&bug=35570&attachment=173916 > https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter. > html&bug=35570&attachment=173917 > https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter. > html&bug=35570&attachment=173918 > > Up to RM what to pick and push but it's a bit of a shame we're fixing the > same things in different places and overlapping in time investment. If my > follow-ups on bug 35570 have gone unnoticed then please let me know what I > should've done differently. > > I'm on board with everything Owen is suggesting, and added my sign-off to > his patch. > Please let me know what else is required of me to help move this. I am sorry, that one is on me. It's hard to keep all things in mind. What helps so that I don't miss a follow-up is to set it back to PQA. That way I WILL stumble upon it again next time I push usually and it won't be missed for sure. I'll try to see if I can resolve any possible conflicts.
Fixed some more "Please Note". This here is not correctI bleive with the mix of concatentation and __() + $('#custom-warning').text(__('The name "' + val + '" is not permitted')).show(); Can you please follow-up on this?
(In reply to Katrin Fischer from comment #61) > Fixed some more "Please Note". > > This here is not correctI bleive with the mix of concatentation and __() > > + $('#custom-warning').text(__('The name "' + val + '" is not > permitted')).show(); > > Can you please follow-up on this? To verify I did a gulp po:update --lang de-DE cd misc/translator/po git grep "is not permitted" but it didn't show up.
(In reply to Owen Leonard from comment #55) > Is it correct that I can submit a request without any fields like title, > author, etc. filled in? Can you please check on that?
Pushed for 24.11! Well done everyone, thank you!
(In reply to Katrin Fischer from comment #63) > (In reply to Owen Leonard from comment #55) > > Is it correct that I can submit a request without any fields like title, > > author, etc. filled in? > > Can you please check on that? This is correct, all fields are optional by design. This is due to the fact that the patron submitting the ILL request may only know the title of what they're looking for, or only know the DOI, etc. This logic can be revisited of course but I believe this is the reasoning.
(In reply to Katrin Fischer from comment #62) > (In reply to Katrin Fischer from comment #61) > > Fixed some more "Please Note". > > > > This here is not correctI bleive with the mix of concatentation and __() > > > > + $('#custom-warning').text(__('The name "' + val + '" is not > > permitted')).show(); > > > > Can you please follow-up on this? > > To verify I did a gulp po:update --lang de-DE > cd misc/translator/po > git grep "is not permitted" but it didn't show up. I may be wrong, but I think this will never work, because gulpfile.js is extracting messages from the following dirs: 'koha-tmpl/intranet-tmpl/prog/js/vue/**/*.vue', 'koha-tmpl/intranet-tmpl/prog/js/**/*.js', 'koha-tmpl/opac-tmpl/bootstrap/js/**/*.js', And shared.js we're talking about here is located in: 'Koha/ILL/Backend/shared-includes/shared.js' I've filed bug 38340 a few days ago for this. These files in Standard backend are designed as DRY, to use the same form logic and javscript for OPAC+Staff. I think the quick fix would be to duplicate these assets and move each set to their respective koha-tmpl/intranet|opac-tmpl directories, but this would be duplicating code. Ideally, with bug 35716, we could move these but not have them duplicated, but I don't think bug 35716 is trivial. Thoughts?
Created attachment 174454 [details] [review] Bug 36221: (QA follow-up): Fix translation entry concat
(In reply to Katrin Fischer from comment #62) > (In reply to Katrin Fischer from comment #61) > > Fixed some more "Please Note". > > > > This here is not correctI bleive with the mix of concatentation and __() > > > > + $('#custom-warning').text(__('The name "' + val + '" is not > > permitted')).show(); > > > > Can you please follow-up on this? > > To verify I did a gulp po:update --lang de-DE > cd misc/translator/po > git grep "is not permitted" but it didn't show up. Added a follow-up for this, though I believe my last comment still stands i.e. this will not add an entry to the po file by running the gulp command.
(In reply to Pedro Amorim from comment #66) > (In reply to Katrin Fischer from comment #62) > > (In reply to Katrin Fischer from comment #61) > > > Fixed some more "Please Note". > > > > > > This here is not correctI bleive with the mix of concatentation and __() > > > > > > + $('#custom-warning').text(__('The name "' + val + '" is not > > > permitted')).show(); > > > > > > Can you please follow-up on this? > > > > To verify I did a gulp po:update --lang de-DE > > cd misc/translator/po > > git grep "is not permitted" but it didn't show up. > > I may be wrong, but I think this will never work, because gulpfile.js is > extracting messages from the following dirs: > 'koha-tmpl/intranet-tmpl/prog/js/vue/**/*.vue', > 'koha-tmpl/intranet-tmpl/prog/js/**/*.js', > 'koha-tmpl/opac-tmpl/bootstrap/js/**/*.js', > > And shared.js we're talking about here is located in: > 'Koha/ILL/Backend/shared-includes/shared.js' > > I've filed bug 38340 a few days ago for this. These files in Standard > backend are designed as DRY, to use the same form logic and javscript for > OPAC+Staff. I think the quick fix would be to duplicate these assets and > move each set to their respective koha-tmpl/intranet|opac-tmpl directories, > but this would be duplicating code. > Ideally, with bug 35716, we could move these but not have them duplicated, > but I don't think bug 35716 is trivial. Thoughts? I think a quick fix for stable releases would be good as this also affects the OPAC (= end users) and then we need to work on a proper fix for the shared assets for 25.05 maybe?
Thanks Pedro! Picked last follow-up "Fix translation entry concat" for main. Small glitch: I have reverted the quotes. Strings need to be in "", so that languages like French using ' inside strings don't break translations (see coding guidelines).
reverted = switched: if (core.indexOf(val.toLowerCase()) > -1) { $('#custom-warning').text(__("The name '%s' is not permitted").format(val)).show(); ...
Thanks Katrin and Owen for the additional findings. (In reply to Katrin Fischer from comment #50) > QA script was not happy for me - could it be a change from another bug > changing things? I was using a ktd image that has user id issues and it somehow makes at least that one check fail silently! :o Very confident having this error is a false positive. The includes and the blocks defined inside are findable. (In reply to Owen Leonard from comment #57) > Should we be using [% PROCESS options_for_libraries %] anywhere there is a > library dropdown? Maybe in general. Though here, there is already WRAPPER ill_select_field that takes care of not having code redundancy here.
(In reply to Katrin Fischer from comment #70) > Small glitch: I have reverted the quotes. Strings need to be in "", so that > languages like French using ' inside strings don't break translations (see > coding guidelines). Oh no, almost all stuff above are actually follow-ups for bug 35570. But the issue about simple quotes also applies to changes from this very bug! - '<input type="text" class="custom-name" name="custom_key">' + - '<input type="text" id="custom-value" name="custom_value"> '+ + '<input type="text" class="custom-name ' + + ( opac ? 'form-control input-fluid custom-field-input' : '') + + '" name="custom_key" placeholder="'+__('key')+'">' + + ' ' + + '<input type="text" id="custom-value" name="custom_value" class="' + + ( opac ? 'form-control input-fluid custom-field-input' : '') +'" placeholder="'+__('value')+'"> ' + It might be tricky with the HTML attribute values being double quoted. And maybe Bug 13768 is relevant.
Enhancement, no 24.05.x backport