Adjust priority of Select2 language picker mechanism #5468
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deltoss
changed the title
deltoss
changed the title
Adjustments for a long line to be shorter.
|
I'm marking this as "needs additional work" since I'd prefer to see a test for this before accepting it. But the code change looks correct and I can add the test before I merge this if needed. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
bump |
kevin-brown
previously requested changes
Jul 17, 2019
…" object, rather then "$.fn.select2.defaults"
|
Thanks for making those changes! This pull request is on track to land in 4.0.9. If you're feeling adventurous, this would benefit from the creation of a test case or two to ensure that we don't break this in the future. You should be able to use the |
|
I think I'll try to attempt that sometime this week. Got the latest updated develop branch, and it looks like I can now run |
Added unit tests for language detection and language detection priorities.
|
Added the tests as per recommendation. Please review, any feedback is appreciated. 👍 |
3 tasks
kevin-brown
added a commit
that referenced
this pull request
Aug 4, 2019
It was pointed out in #5468 that the `lang` attribute, when inherited from a parent element, was above the option set in the global default within the inheritance chain. While this makes sense because of how we inherit other properties, it does not make sense for the `lang` attribute. The inheritance chain for the `language` option has been adjusted to be the following: 1. The `lang` attribute on the original `<select>` element 2. The `data-language` attribute on the original `<select>` element (because of how `data-*` attribute resolution affects options) 3. The `language` option specified when initiailizing Select2 4. The `language` Select2 default 5. The `lang` attribute on a parent of the `<select>` element While this is a breaking change, we believe that this change will have minimal to no impact on real-world usage of Select2, because of how the `lang` attribute is generally used within documents. We believe this will now make setting the default language through JavaScript easier and more reliable, bringing it in line with how we recommend it is done within the documentation. This was implemented through a new method `Defaults.applyFromElement` instead of within the old `Options.fromElement` method because it relies on the global defaults object. While we could have reached in to the internals in order to apply it appropriately, it made more sense to handle the proper resolution through a single consistent place. This was not implemented in the `Defaults.apply` method because that method is not typically passed in a reference to the original `<select>` element that it is applying the options for. Closes #5468
kevin-brown
added a commit
that referenced
this pull request
Aug 4, 2019
It was pointed out in #5468 that the `lang` attribute, when inherited from a parent element, was above the option set in the global default within the inheritance chain. While this makes sense because of how we inherit other properties, it does not make sense for the `lang` attribute. The inheritance chain for the `language` option has been adjusted to be the following: 1. The `lang` attribute on the original `<select>` element 2. The `data-language` attribute on the original `<select>` element (because of how `data-*` attribute resolution affects options) 3. The `language` option specified when initiailizing Select2 4. The `language` Select2 default 5. The `lang` attribute on a parent of the `<select>` element While this is a breaking change, we believe that this change will have minimal to no impact on real-world usage of Select2, because of how the `lang` attribute is generally used within documents. We believe this will now make setting the default language through JavaScript easier and more reliable, bringing it in line with how we recommend it is done within the documentation. This was implemented through a new method `Defaults.applyFromElement` instead of within the old `Options.fromElement` method because it relies on the global defaults object. While we could have reached in to the internals in order to apply it appropriately, it made more sense to handle the proper resolution through a single consistent place. This was not implemented in the `Defaults.apply` method because that method is not typically passed in a reference to the original `<select>` element that it is applying the options for. Closes #5468
|
Thanks for adding the tests! While reading and reviewing this pull request a week ago, I came to the conclusion that the translation system for Select2 was broken and should really be fixed. Unfortunately that meant that this pull request was basically replaced by #5602, so I'm going to have to close it off. The good news though is that the bug originally described in this pull request has been fixed and will be included in the 4.0.9 release. I'm sorry for making you go through the work of correcting this pull request based on the feedback that was given. Especially considering the long amount of time that it took to initially review this in the first place. |
kevin-brown
added a commit
that referenced
this pull request
Aug 14, 2019
* Made the test suite for translations more complete There were previously tests for translations within the test suite, but they only covered a select few problem cases that had previously been fixed. They did not cover the majority of cases, which makes changes to how the translation mechanism for Select2 works a bit more challenging. So this adds tests for the majority of edge cases around translations, including how one would expect the fallback chains to work and also around how defaults interact with the language options. This should not be considered an exhaustive list of all of the edge cases, but it should be good enough to refactor the internals and not have to worry as much. The one change of note to this test file is that we are now properly resetting the defaults in between tests. This should fix any issues that we may have seen where the defaults were not being reset, and thus tests were not properly isolated and would start to interfere with each other. This required pulling the module definition down below the imports, since we need to reference the defaults within the module definition. Many of these tests will fail because the translation system is broken in many small, unrealized ways. The next few commits should make these pass and fix the issues that we are seeing. * Consistently resolve the language option This fixes an issue that we have had for a while where we did not have a way to consistently take the `language` option from a string, array, object, or whatever else is specified and resolve it into `Translation`-compatible objects or strings. Now there is a new internal `_resolveLanguage` function which is able to take any of the supported ways of specifying a language and resolve it down into the supported language chain. This now means that we can properly resolve the following cases, and we can do it in a consistent manner. * When the language is specified as just a string (for example: "en") * When the language is specified as a string containing a region (for example: "en-US") * When the langugae chian is specified as a list of strings (for example: ["es", "en"]) * When the language is specifid as an object containing messages (for example, when a user overrides only a subset of messages) * When the language is specified as a list of strings and objects (for example, when a user wants to use a language other than English and also wants to ovverride some default messages) * When the language is not specified at all (the most common case) * When the language is specified as an empty object (an edge case that allows us to skip processing it) This allows us to consistently produce the language fallback chain based on the given `language` option, something which we could not actually do before because we didn't have a consistent chain. This also means that now the `language` option will consistently be an array after going through this process, instead of being any number of types before. The translation generation currently does not support having objects and strings mixed as a part of the fallback chain, despite that being how the default chain has always worked, and as such there are still failing tests around this. * Move English to always be at the end of the language chain This was technically true in most cases in the past, because if a language chain was manually specified then it would have English injected into the end of it anyway. This is needed because not all translations are complete, but we know the English one is, and Select2 relies on the translation that it uses being complete. This will result in cases where a user specifies a language but still receives English translation for some things, which is what users have historically seen when using partial translations anyway. This just ensures that there will always be a complete translation that is being used, so they won't get unexpected errors and will instead get unexpected English translations. * Filter out repeated languages in fallback chain This is mostly being done for performance reasons, since Select2 will not behave any differently when there are duplicates, but it makes things cleaner when you ask for the fallback chain and it only contains unique values. This cannot distinguish between languages specified by name (string) and languages specified by the contents of their language file (such as the default, English), but this should generally not be an issue. * Convert the language chain into a finalized translation This extracts the logic for converting parts of the language chain into the finalized `Translation` objects out into its own method, with some small fixes for edge cases. This can now properly convert a language chain containing both strings and objects into a translation object that contains them both. We no longer need to special case the `language` option being an array since we know that it will be an array once the language resolution process is completed. * Switch default translation to be empty This should have no external effects, but it fixes an interesting bug where resetting the defaults would not always reset custom translations. This was because it was possible to modify the included English translation when you were setting a default for the language option. This should not cause any issues because the English translation is now appended to the end of the language chain when the defaults are applied, which means that English will continue to exist as the final fallback. * Inherited `lang` attribute should be below the default in the chain It was pointed out in #5468 that the `lang` attribute, when inherited from a parent element, was above the option set in the global default within the inheritance chain. While this makes sense because of how we inherit other properties, it does not make sense for the `lang` attribute. The inheritance chain for the `language` option has been adjusted to be the following: 1. The `lang` attribute on the original `<select>` element 2. The `data-language` attribute on the original `<select>` element (because of how `data-*` attribute resolution affects options) 3. The `language` option specified when initiailizing Select2 4. The `language` Select2 default 5. The `lang` attribute on a parent of the `<select>` element While this is a breaking change, we believe that this change will have minimal to no impact on real-world usage of Select2, because of how the `lang` attribute is generally used within documents. We believe this will now make setting the default language through JavaScript easier and more reliable, bringing it in line with how we recommend it is done within the documentation. This was implemented through a new method `Defaults.applyFromElement` instead of within the old `Options.fromElement` method because it relies on the global defaults object. While we could have reached in to the internals in order to apply it appropriately, it made more sense to handle the proper resolution through a single consistent place. This was not implemented in the `Defaults.apply` method because that method is not typically passed in a reference to the original `<select>` element that it is applying the options for. Closes #5468 * Properly resolve language chains with dictionaries It is possible for a language chain to be specified that includes both a dictionary and the string translation as a part of the same chain, but we would previously throw an error because we assumed that the list could only contain strings and that dictionaries would never be included in lists. This fixes the issue so language region normalization only occurs when a string is specified in the language chain, which is what we were previously assuming was the case but was not actually. This also now resolves the entire language option during the `Defaults.apply` method. This should be a no-op except for internal tests, because the `Defaults.applyFromElement` method should almost always be called in real-world scenarios.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request includes a
I noticed that Select2 internationalisation mechanisms picks up the language in this priority:
languageoptionlangproperty of the select elementlang$.fn.select2.defaults.set('language', '<LANGUAGE>');I really think 3 and 4 should be switched around, especially since the HTML language code doesn't perfectly match up with Select2, looking at the list at (W3Schools).[https://www.w3schools.com/tags/ref_language_codes.asp].
For example, Select2 has zh-CN, which means traditional chinese, but in the above language code, it's actually zh-Hans. Thus, it's a good idea to switch the order of
3and4around so it'll look like:languageoptionlangproperty of the select element$.fn.select2.defaults.set('language', '<LANGUAGE>');langFor more information, see:
Select2 Issue post
Also... I can't seem to do an
npm installon select2... so I couldn't compile, minify and test according to theCONTRIBUTING.mdguide. I get the error below.Thus, all I checked in was the changes to the source file. I did test it through trying it out on the browser with examples however.