-
-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tab adjustments: Sort the translated optionlist #3152
Conversation
This comment has been minimized.
This comment has been minimized.
AUTOMERGE: (FAIL)
|
This comment has been minimized.
This comment has been minimized.
src/js/tabs/adjustments.js
Outdated
@@ -286,6 +286,14 @@ adjustments.cleanup = function (callback) { | |||
if (callback) callback(); | |||
}; | |||
|
|||
$.fn.sortSelect = function() { | |||
const op = this.children("option"); | |||
op.sort(function(a, b) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to use a locale compare to be sure to order them correctly in any language? For example like here:
betaflight-configurator/src/js/tabs/osd.js
Line 2644 in bc6307a
if ($(this).text().localeCompare(field.text(), currentLocale, { sensitivity: 'base' }) > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I give it a try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also make it into a common util function as we might need it in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the other reviewers, any chance this can be done as locale specific and in a common function?
I'll stiil want to give it a try, but had recently spent som time at an EdgeTx PR :-) |
Have changed the When it's merged you only have to remove the local function and change the call
to keep |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Do you want to test this code? Here you have an automated build: |
@haslinghuis, thank you for #3167, I was in fact stucked in try to use sortElement with language sort, but that spoiled the sort used in other places (as you now have changed). |
By the way, this PR require #3167 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but waiting for the other PR to be merged.
Thank you |
When the optionlist are translated to non english, the order was chaotic, now ordered by lexcial sort of translated text