Closed Bug 1209135 Opened 6 years ago Closed 6 years ago

[Music][NGA] Switch sorting logic to use String.prototype.localeCompare()


(Firefox OS Graveyard :: Gaia::Music, defect)

Gonk (Firefox OS)
Not set


(Not tracked)



(Reporter: justindarc, Assigned: squib)




(1 file)

For a better l10n experience, String.prototype.localeCompare() should be used. This should provide correct, locale-specific sorting  and should also sort numbers correctly (e.g. 1, 2, ... 10 vs. 1, 10, 2, ... ).
Assigning to :squib.

Setting NI? for :gandalf regarding usage of String.prototype.localeCompare(). Do we need to get the current locale somehow to use String.prototype.localeCompare()? Or does it automatically detect the current locale?
Assignee: nobody → squibblyflabbetydoo
Depends on: 1208154
Flags: needinfo?(gandalf)
The pattern we use is to pass `navigator.languages` to it. In the future we may want to pass document.l10n.languages or sth similar (the difference being - navigator.languages knows languages requested by the user, document.l10n.languages would know languages that were resolved by language negotiation and ones that we actually use).

The interesting bits for you will be about IntlHelper. If you compare in a loop, using localeCompare will not be fast [0].

It's better to create an Intl.Collator object and then use it on the strings.

IntlHelper will help you cache the collator and allow you to easily define the observer on a function to be fired when the object is reset. (Currently we reset Collator only on languageschange, but we may add more triggers in the future).

Flags: needinfo?(gandalf)
Duplicate of this bug: 1074542
Duplicate of this bug: 1090488
Duplicate of this bug: 963254
Duplicate of this bug: 1088860
Duplicate of this bug: 813871
Attachment #8669095 - Flags: review?(jdarcangelo)
Duplicate of this bug: 1210684
Comment on attachment 8669095 [details] [review]
[gaia] jimporter:music-sort > mozilla-b2g:master

Overall looks good! Nice cleanup as well. See my nits in the PR as well as some directions for rebasing the additional script loading.
Attachment #8669095 - Flags: review?(jdarcangelo) → review+
Forgot to resolve this. Landed:
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.