Stores alphabetical sorting #345
Conversation
The bulk of this small change was to sort stores by alphabetical order according to translated names, instead of english strings. This meant removing the ID splitting as well as hyphen/space replacing, as the ID is not considered for the test. Also changed: 1. line 800: as the original english string is no longer necessary in the iteration, k takes the value of translated string. k is then used for alphabetical comparison (as before) with the right value. 2. line 816: removed the test on curPrev variable. Items already listed have been placed in alphabetical order, so there is no need to check whether each item actually follows value of control variable.
|
+1 |
| @@ -797,6 +797,7 @@ var Room = { | |||
| $SM.set('stores["'+k+'"]', 0); | |||
| } | |||
|
|
|||
| k = _(k); | |||
There was a problem hiding this comment.
I think I'd prefer to use a different variable here, since k is used for object lookups and stuff. If someone moves this line, or adds more lookups below it (thinking it's fine, 'cause we do it above), things will break unexpectedly.
There was a problem hiding this comment.
I will fix it right now.
However, I did the same for outside.js (in that case it's a variable called "name"). I saw that you merged my change, but perhaps I should have used something different there, too, for consistency. I'll give it a try.
There was a problem hiding this comment.
I don't think that one was being used for lookups, was it? If so, I should
have caught it too...
On Thu, Apr 30, 2015 at 10:48 AM Andrea Rendine notifications@github.com
wrote:
In script/room.js
#345 (comment)
:@@ -797,6 +797,7 @@ var Room = {
$SM.set('stores["'+k+'"]', 0);
}
k = _(k);I will fix it right now.
However, I did the same for outside.js (in that case it's a variable
called "name"). I saw that you merged my change, but perhaps I should have
used something different there, too, for consistency. I'll give it a try.—
Reply to this email directly or view it on GitHub
https://github.com/doublespeakgames/adarkroom/pull/345/files#r29435743.
"k" still refers to the English string, while "lk" carries the translated value.
|
Variable name fixed (k into lk, what a fantasy). Also posted #349 for "name" variable in outside.js. No, "name" is not used for lookups, now, but if in future changes updateVillageRow() is extended, it could be useful to split original and translated names. |
| @@ -797,6 +797,7 @@ var Room = { | |||
| $SM.set('stores["'+k+'"]', 0); | |||
| } | |||
|
|
|||
| lk = _(k); | |||
There was a problem hiding this comment.
Needs var or you're polluting the global scope. Polluting the global scope is bad.
|
Fixed var. Sorry for the mess. |
|
No problem at all! Thanks for helping out. |
Stores alphabetical sorting
|
Thank you for the invaluable job in developing ADR! |
Fixes #185 for stores box.