Repository navigation
Closes #9418: Admin UI: sort the realm localization texts alphabetically - #9419
Conversation
There was a problem hiding this comment.
Thanks for contributing this change, @leischt. I tested it locally and it works fine. Great work!
Looking at the code formatting, I see that a blank line slipped in, probably unintended. Please remove that one blank line and then I'll approve this PR.
Please keep it as one commit and squash your new changes with the previous commit. Thanks!
There was a problem hiding this comment.
A surplus blank line slipped in here, please remove it.
| }) | |
| } | |
| }) | |
| } |
There was a problem hiding this comment.
@ahus1 thanks for the review. Sorry this new line was indeed unintended. I updated the commit accordingly.
727eb6a to
7652487
Compare
There was a problem hiding this comment.
I tested the changes locally and they worked for me. I also reviewed the code changes and they look good to me.
There was a problem hiding this comment.
Arrow functions are ES2015 syntax, and to my knowledge we're not transpiling this code to older versions of JavaScript. Since we still have to support older browsers for the Admin UI I believe this could break the code there.
There is also no guarantee that the properties of an object will remain in insertion order when iterating on them. See the definition of an Object from ECMAScript Third Edition (pdf):
4.3.3 8000 Object
An object is a member of the type Object. It is an unordered collection of properties each of which contains a primitive value, object, or function. A function stored in a property of an object is called a method.
This means that we would have to use some kind of type here like Array or Set to ensure that this remains in the correct order.
There was a problem hiding this comment.
Hi @jonkoops - thanks for looking at this and finding this I didn't. "+1" for the arrow functions.
As I was wondering why my manual test succeeded while the spec says this is no guaranteed behavior, I found this discussion on SO indicating that the specified behavior changed in ES6 (see answer by Mathias Bynens): https://stackoverflow.com/questions/5467129/sort-javascript-object-by-key
As you point out that this should run on older browsers as well, this must not be relied upon. A quick look at the docs didn't reveal a list of supported browsers to me. Could you please point me at more information about this topic? Thanks!
There was a problem hiding this comment.
I do not believe we currently have the browser support publicly documented, but the worst case browser we have to support at the moment is Internet Explorer 11. For the rest of the browsers the target is pretty evergreen.
In the new Admin UI we're targeting only evergreen browsers.
There was a problem hiding this comment.
Thank you so much for your feedback.
I was not aware of these constraints and updated the PR to not use an Object anymore.
Since ng-repeat does not support Map iteration, I used an Array instead.
Further feedback is very welcome.
There was a problem hiding this comment.
Great! I think using an array here is an excellent option considering the browser constraints.
7652487 to
bdc9773
Compare
There was a problem hiding this comment.
This could possibly also be a reduce() operation.
| const array = []; | |
| for(let i = 0; i < keys.length; i++){ | |
| if(typeof object[keys[i]] === 'string') { | |
| array.push([keys[i], object[keys[i]]]); | |
| } | |
| } | |
| return array; | |
| return keys.reduce(function (result, key) { | |
| return result.concat([[key, object[key]]]); | |
| }, []); |
There was a problem hiding this comment.
I omitted the type check here (not sure if there are non-string values here), but if you'd want to include it you could do the following.
return keys.reduce(function (result, key) {
const value = object[key];
if (typeof value !== 'string') {
return result;
}
return result.concat([[key, value]]);
}, []);There was a problem hiding this comment.
Calling sort() like this means that the values will be sorted by the UTF-16 code units order value of the string (see MDN documentation). This means that certain words might not be sorted in what the user would expect, for example if a string contains uppercase letter or special characters (e.g. réservé or RESERVE).
To mitigate this the String prototype has a localeCompare() method that can be used to compare strings in a manner that the user would expect them to be ranked in their respective locale.
| const keys = Object.keys(object).sort(); | |
| const keys = Object.keys(object).sort(function (a, b) { | |
| return a.localeCompare(b, 'en-US'); | |
| }); |
In this example en-US should be replaced with the current locale the user is using in the Admin UI.
bdc9773 to
6adfe15
Compare
|
@jonkoops Thank you so much for the detailed feedback. I pushed a new version, which includes your suggestions. |
There was a problem hiding this comment.
Reviewed it and tested it locally with some German umlauts as keys. Works great - thanks a lot for everyone involved here!
|
@ssilvert could you restart the build? It looks like there was random connection error that caused it to fail. |
I restarted yesterday and got the same error. Trying one more time. |
|
The way to proceed is to just merge it. I don't see any real risk so I've done just that. |
No description provided.