|
Description
Jonathan Druart
2020-06-12 14:23:01 UTC
Created attachment 105822 [details] [review] Bug 25727: Upgrade the Select2 JS lib to version 4 In order to benefit from the last feature of Select we should upgrade to the version 4. For instance it adds the ability to dynamically create new entries https://select2.org/tagging Test plan: Confirm that there is no visible regression on dropdown list with select2 Like edit a bibliographic records, and the items. Sponsored-by: Orex Digital I see some issues with this when I compare the behavior in master to the behavior when this patch is applied: - In master when there are no entries in the dropdown it shows a message in the dropdown, "No matches found." With the new version there is no message. - Looking at the basic MARC editor, testing a required subfield (in my test, 952c). Submitting the form causes the HTML <select> to appear above the Select2 control - The old version let the user select an empty option by clicking an "x" in the control. The new version includes an empty option just like an HTML select would. I think both are reasonable but I wonder if this is something we want to intentionally change. Created attachment 105853 [details] [review] Bug 25727: Adjust translations Created attachment 105854 [details] [review] Bug 25727: Fix default values The way we defined the default values does not longer work (?) Using $.fn.select2.defaults.set instead of jQuery.extend($.fn.select2.defaults, { fixes the problem. It restores the allowClear setting. There is a change in the behavior here, the selection opens when the clear button is clicked. That is coming from a change in Select2 apparently. We may want to hack something to restore the precedent behaviour. This patch also removes the redefinition of the width. It seems that the issue that this code was trying to correct is now fixed. Created attachment 105855 [details] [review] Bug 25727: Fix highlight of a missing field In the previous version of Select2 we had only 1 'a' tag that was visible, next to the invisible select. Now we have a 3 nested span elt, the last one (with a role=combobox attribute) is the one we want to style. (In reply to Owen Leonard from comment #2) Thanks Owen! > I see some issues with this when I compare the behavior in master to the > behavior when this patch is applied: > > - In master when there are no entries in the dropdown it shows a message in > the dropdown, "No matches found." With the new version there is no message. Indeed. There was a translation issue I have "fixed" now, however the behaviour is still the same. I think it is the expected one, but I have not looked at Select2 code. https://select2.org/upgrading/migrating-from-35#renamed-translation-options See the last patch. I found the list there: https://github.com/select2/select2/blob/develop/dist/js/i18n/en.js I think it's better to continue handling the translation on our side. > - Looking at the basic MARC editor, testing a required subfield (in my > test, 952c). Submitting the form causes the HTML <select> to appear above > the Select2 control Fixed. > - The old version let the user select an empty option by clicking an "x" in > the control. The new version includes an empty option just like an HTML > select would. I think both are reasonable but I wonder if this is something > we want to intentionally change. Restored in the patch "Fix default values" Created attachment 105975 [details] [review] Bug 25727: Upgrade the Select2 JS lib to version 4 In order to benefit from the last feature of Select we should upgrade to the version 4. For instance it adds the ability to dynamically create new entries https://select2.org/tagging Test plan: Confirm that there is no visible regression on dropdown list with select2 Like edit a bibliographic records, and the items. Sponsored-by: Orex Digital Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 105976 [details] [review] Bug 25727: Adjust translations Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 105977 [details] [review] Bug 25727: Fix default values The way we defined the default values does not longer work (?) Using $.fn.select2.defaults.set instead of jQuery.extend($.fn.select2.defaults, { fixes the problem. It restores the allowClear setting. There is a change in the behavior here, the selection opens when the clear button is clicked. That is coming from a change in Select2 apparently. We may want to hack something to restore the precedent behaviour. This patch also removes the redefinition of the width. It seems that the issue that this code was trying to correct is now fixed. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 105978 [details] [review] Bug 25727: Fix highlight of a missing field In the previous version of Select2 we had only 1 'a' tag that was visible, next to the invisible select. Now we have a 3 nested span elt, the last one (with a role=combobox attribute) is the one we want to style. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 105979 [details] [review] Bug 25727: (follow-up) Use CSS specificity rather than !important This patch adds specificty to the CSS which controls the "invalid" appearance of a Select2 dropdown so that the use of "!important" can be avoided. Created attachment 105980 [details] [review] Bug 25727: (follow-up) Use CSS specificity rather than !important This patch adds specificty to the CSS which controls the "invalid" appearance of a Select2 dropdown so that the use of "!important" can be avoided. The patch also puts this style into our custom select2 CSS file so that it is located with other Select2 style changes. Thanks for the follow-up Owen. I did not do that because I though it was too tight with the select2 classes' names, but if you think it's the way to go, I am with you. However, should not we removed the definition in the staff-global.css then? Created attachment 108351 [details] [review] Bug 25727: Upgrade the Select2 JS lib to version 4 In order to benefit from the last feature of Select we should upgrade to the version 4. For instance it adds the ability to dynamically create new entries https://select2.org/tagging Test plan: Confirm that there is no visible regression on dropdown list with select2 Like edit a bibliographic records, and the items. Sponsored-by: Orex Digital Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 108352 [details] [review] Bug 25727: Adjust translations Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 108353 [details] [review] Bug 25727: Fix default values The way we defined the default values does not longer work (?) Using $.fn.select2.defaults.set instead of jQuery.extend($.fn.select2.defaults, { fixes the problem. It restores the allowClear setting. There is a change in the behavior here, the selection opens when the clear button is clicked. That is coming from a change in Select2 apparently. We may want to hack something to restore the precedent behaviour. This patch also removes the redefinition of the width. It seems that the issue that this code was trying to correct is now fixed. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 108354 [details] [review] Bug 25727: Fix highlight of a missing field In the previous version of Select2 we had only 1 'a' tag that was visible, next to the invisible select. Now we have a 3 nested span elt, the last one (with a role=combobox attribute) is the one we want to style. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 108355 [details] [review] Bug 25727: (follow-up) Use CSS specificity rather than !important This patch adds specificty to the CSS which controls the "invalid" appearance of a Select2 dropdown so that the use of "!important" can be avoided. The patch also puts this style into our custom select2 CSS file so that it is located with other Select2 style changes. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 1) I think the new behaviour of the x button that no longer clears the field is confusing and will cause a lot of questions in support. It did not always work nicely for me on the first go to select the 'empty' entry either. You mention in one of the comments that we might want to fix this and I think we should. 2) The second thing is that the sizes are slightly different. For example on the edit item form the location "New materials shelf" fits into one line before, but not after. I'd like to see the input fields adapt better to the length of the entries as this makes things harder to read. (Coming from someone whose language tends to have loooong words). Created attachment 108395 [details] [review] Bug 25727: Do not open options on clear We want to restore the previous behaviour and keep the options closed when clear is clicked https://github.com/select2/select2/issues/3320#issuecomment-621662039 1. is fixed, for 2 I can take a look later. Today I am facing problems with yarn and cannot regenerate the css file. Created attachment 108400 [details] [review] Bug 25727: Default to dropdownAutoWidth This will set a correct width by default. And this patch seems to fix 2! Can you confirm? I think we are not quite there yet - location is ok now, but "Institut Protestant de Théologie" in the libraries list still wraps on 2 lines. I think they should probably adjust their width dynamically? Created attachment 108436 [details] [review] Bug 25727: Do not wrap option text Katrin, this is all what I managed to do. It's not perfect and I am not sure you will be happy with it.. Ohoh. Would it make sense to ask others to take a look? I remember we resolved another tricky CSS issue latley with more brain power :) Created attachment 108823 [details] [review] Bug 25727: (follow-up) Some style updates, JS i18n This patch makes to general changes. First, it makes some minor style changes to the appearance of the dropdowns. Second, it moves the JS content in select2.inc into a separate JS file. This is made possible by Bug 21156. To test, apply the patch and view a page which uses the Select2 library, e.g. the add item screen. Confirm that the Select2-styled dropdowns look good and work correctly. TESTING TRANSLATABILITY - Update a translation, e.g. fr-FR: > cd misc/translator > perl translate update fr-FR - Open the corresponding .po file for JavaScript strings, e.g. misc/translator/po/fr-FR-messages-js.po - Locate strings pulled from koha-tmpl/intranet-tmpl/prog/js/select2.js for translation, e.g.: msgid "No results found" msgstr "" - Edit the "msgstr" string however you want (it's just for testing). - Install the updated translation: > perl translate install fr-FR - Switch to your newly translated language in the staff client and test a Select2-styled dropdown. Test that translated strings appear. For example: - Use the filter form to search for a string which isn't found. You should see a translated version. - Make a selection in one of the dropdowns. Hover your mouse over the "X" icon which now appears. The tooltip should be a translated version. > Bug 25727: Do not wrap option text
My patch excludes this one. I think it's preferable to have the option text wrap rather than have horizontal scrollbars.
I tried but was unable to improve the width of the dropdowns. I consider it a bug in this version of Select2 because I don't think there is a configuration option that will solve it.
Created attachment 108829 [details] [review] Bug 25727: (follow-up) Some style updates, JS i18n This patch makes to general changes. First, it makes some minor style changes to the appearance of the dropdowns. Second, it moves the JS content in select2.inc into a separate JS file. This is made possible by Bug 21156. To test, apply the patch and view a page which uses the Select2 library, e.g. the add item screen. Confirm that the Select2-styled dropdowns look good and work correctly. TESTING TRANSLATABILITY - Update a translation, e.g. fr-FR: > cd misc/translator > perl translate update fr-FR - Open the corresponding .po file for JavaScript strings, e.g. misc/translator/po/fr-FR-messages-js.po - Locate strings pulled from koha-tmpl/intranet-tmpl/prog/js/select2.js for translation, e.g.: msgid "No results found" msgstr "" - Edit the "msgstr" string however you want (it's just for testing). - Install the updated translation: > perl translate install fr-FR - Switch to your newly translated language in the staff client and test a Select2-styled dropdown. Test that translated strings appear. For example: - Use the filter form to search for a string which isn't found. You should see a translated version. - Make a selection in one of the dropdowns. Hover your mouse over the "X" icon which now appears. The tooltip should be a translated version. Created attachment 108830 [details] [review] Bug 25727: (follow-up) Some style updates, JS i18n This patch makes to general changes. First, it makes some minor style changes to the appearance of the dropdowns. Second, it moves the JS content in select2.inc into a separate JS file. This is made possible by Bug 21156. To test, apply the patch and view a page which uses the Select2 library, e.g. the add item screen. Confirm that the Select2-styled dropdowns look good and work correctly. TESTING TRANSLATABILITY - Update a translation, e.g. fr-FR: > cd misc/translator > perl translate update fr-FR - Open the corresponding .po file for JavaScript strings, e.g. misc/translator/po/fr-FR-messages-js.po - Locate strings pulled from koha-tmpl/intranet-tmpl/prog/js/select2.js for translation, e.g.: msgid "No results found" msgstr "" - Edit the "msgstr" string however you want (it's just for testing). - Install the updated translation: > perl translate install fr-FR - Switch to your newly translated language in the staff client and test a Select2-styled dropdown. Test that translated strings appear. For example: - Use the filter form to search for a string which isn't found. You should see a translated version. - Make a selection in one of the dropdowns. Hover your mouse over the "X" icon which now appears. The tooltip should be a translated version. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 108875 [details] [review] Bug 25727: Upgrade the Select2 JS lib to version 4 In order to benefit from the last feature of Select we should upgrade to the version 4. For instance it adds the ability to dynamically create new entries https://select2.org/tagging Test plan: Confirm that there is no visible regression on dropdown list with select2 Like edit a bibliographic records, and the items. Sponsored-by: Orex Digital Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108876 [details] [review] Bug 25727: Adjust translations Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108877 [details] [review] Bug 25727: Fix default values The way we defined the default values does not longer work (?) Using $.fn.select2.defaults.set instead of jQuery.extend($.fn.select2.defaults, { fixes the problem. It restores the allowClear setting. There is a change in the behavior here, the selection opens when the clear button is clicked. That is coming from a change in Select2 apparently. We may want to hack something to restore the precedent behaviour. This patch also removes the redefinition of the width. It seems that the issue that this code was trying to correct is now fixed. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108878 [details] [review] Bug 25727: Fix highlight of a missing field In the previous version of Select2 we had only 1 'a' tag that was visible, next to the invisible select. Now we have a 3 nested span elt, the last one (with a role=combobox attribute) is the one we want to style. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108879 [details] [review] Bug 25727: (follow-up) Use CSS specificity rather than !important This patch adds specificty to the CSS which controls the "invalid" appearance of a Select2 dropdown so that the use of "!important" can be avoided. The patch also puts this style into our custom select2 CSS file so that it is located with other Select2 style changes. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108880 [details] [review] Bug 25727: Do not open options on clear We want to restore the previous behaviour and keep the options closed when clear is clicked https://github.com/select2/select2/issues/3320#issuecomment-621662039 Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108881 [details] [review] Bug 25727: Default to dropdownAutoWidth This will set a correct width by default. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 108882 [details] [review] Bug 25727: (follow-up) Some style updates, JS i18n This patch makes to general changes. First, it makes some minor style changes to the appearance of the dropdowns. Second, it moves the JS content in select2.inc into a separate JS file. This is made possible by Bug 21156. To test, apply the patch and view a page which uses the Select2 library, e.g. the add item screen. Confirm that the Select2-styled dropdowns look good and work correctly. TESTING TRANSLATABILITY - Update a translation, e.g. fr-FR: > cd misc/translator > perl translate update fr-FR - Open the corresponding .po file for JavaScript strings, e.g. misc/translator/po/fr-FR-messages-js.po - Locate strings pulled from koha-tmpl/intranet-tmpl/prog/js/select2.js for translation, e.g.: msgid "No results found" msgstr "" - Edit the "msgstr" string however you want (it's just for testing). - Install the updated translation: > perl translate install fr-FR - Switch to your newly translated language in the staff client and test a Select2-styled dropdown. Test that translated strings appear. For example: - Use the filter form to search for a string which isn't found. You should see a translated version. - Make a selection in one of the dropdowns. Hover your mouse over the "X" icon which now appears. The tooltip should be a translated version. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Should we file a bug for the width issue to keep open until it can be resolved? (maybe with the next version of select2?) (In reply to Katrin Fischer from comment #40) > Should we file a bug for the width issue to keep open until it can be > resolved? (maybe with the next version of select2?) I think we should reach them. I will do it next week. Pushed to master for 20.11, thanks to everybody involved! (In reply to Jonathan Druart from comment #41) > (In reply to Katrin Fischer from comment #40) > > Should we file a bug for the width issue to keep open until it can be > > resolved? (maybe with the next version of select2?) > > I think we should reach them. I will do it next week. No need for that, see bug 26288. The next version (in beta so far) seems to fix the problem. Created attachment 109359 [details] [review] Bug 25727: (follow-up) Do not open options on clear Created attachment 109361 [details] [review] Bug 25727: (follow-up) Do not open options on clear (In reply to Jonathan Druart from comment #45) > Created attachment 109361 [details] [review] [review] > Bug 25727: (follow-up) Do not open options on clear Pushed to master for 20.11. enhancement will not be backported to 20.05.x |