-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Now multiCombo Languages will show human readable languages instead of ISO codes #11699
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
Conversation
1ec5
left a comment
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.
This is a good start, thank you!
modules/ui/fields/combo.js
Outdated
| let langName = localizer.languageName(tval); | ||
| if (langName) { | ||
| return langName; | ||
| }; |
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.
Nit: stray semicolon.
modules/ui/fields/combo.js
Outdated
| return { | ||
| key: v, | ||
| value: label, | ||
| value: v, |
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.
Due to this change, you can search for the language code “hi” and get Hindi, but you can’t search for “hindi” and get Hindi. This also affects other combo and multicombo boxes. Some users will know the raw code but others (especially non-English speakers) will only know the localized display name. We want the user to be able to search for either. I’m pretty sure the underlying control’s implementation already knows how to search by the key, so value should be label.
|
Sure I will work on the changes and let you know if I face any problem in between. These are the things I have understand I need to work on: Things to do:
Do let me know if there is something I missed. |
|
I was able to do these changes, do let me know if there is something wrong. |
modules/ui/fields/combo.js
Outdated
| options.push({ | ||
| key: 'others', | ||
| value: 'others', | ||
| title: 'others', |
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.
We should allow id-tagging-schema to provide its own localized string for the title of this option. Can we make the initialization of this option more consistent with the generic initialization down below?
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.
Basically we need others to get localized in different languages, I think this can be done if we use stringsField to fill the 'others' option and push in the options array.
modules/ui/fields/combo.js
Outdated
| var k = d.key; | ||
| if (_isMulti) k = k.replace(field.key, ''); | ||
| // Ignore the raw-value class for key language: | ||
| if (field.key === 'language:' && localizer.languageName(k) && k !== 'others') return false; |
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.
This is fine for now, but we’ll probably want to find a more data-driven approach in the future, when we enhance similar fields like Payment Methods, Currency, and Country (#7123).
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.
Yes, we will have to come up with something like specialKey boolean for payment, currency, language, and country or any others, that will be a better long-term solution for these type of keys.
| options.sort((a, b) => { | ||
| return a.value.localeCompare(b.value); | ||
| }); |
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.
String.prototype.localeCompare uses the browser’s language preference, but there are a few other ways for the user to customize iD’s interface language, such as setting ?locale= in the URL. Pass in the locale code to ensure consistency. (Given the number of items to sort, best to store the locale outside of the sort() call.)
iD/modules/ui/sections/preset_fields.js
Lines 88 to 90 in aaaa972
| additionalFields.sort(function(field1, field2) { | |
| return field1.title().localeCompare(field2.title(), localizer.localeCode()); | |
| }); |
|
Ok I will maintain consistency and apply the changes accordingly. |
…, and sorting will be done on the basis of local language
|
I have made the changes, do let me know if somethings wrong. |
1ec5
left a comment
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.
There’s one more styling issue. Once that’s fixed, it’ll be good enough to land.
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.
Looks great, thank you for persisting through multiple iterations! In case you’re interested, #7123 would be a good opportunity for you to expand upon what you’ve built here. I haven’t opened an issue about the Currency field yet, but it’s the same idea there.
Fixes #11652
Changes made:
displayValuefunction now looks at the field.key and if it islanguage:, it will uselocalizer.languageNameontval.renderValuefunction will also look at the field.key and it does the same likedisplayNamecomboDataarray will now havevalue=vso that choosing 'Hindi' will not make tag aslanguage:Hindi=yesand instead it will belanguage:hi=yes.Before can be seen on the issue, here is the after image:
Btw I have a doubt, when we click on the dropdown we see some language only like Hindi is not present but when I type hi, I can see Hindi as an option and can set it, is it intentional?