-
Notifications
You must be signed in to change notification settings - Fork 440
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
fix: Account names should use natural sort order #1663
base: master
Are you sure you want to change the base?
Conversation
4301f84
to
571137c
Compare
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.
Good catch! Could we apply these two suggestions?
const collator = new Intl.Collator(undefined, { | ||
numeric: true, | ||
sensitivity: "base" | ||
}); | ||
|
||
accounts = accounts.sort((a, b) => collator.compare(a.name, b.name)); |
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.
can we move this to AccountService.getAccounts
?
so we fix the issue at the best place where the incorrect order is being returned
fuels-wallet/packages/app/src/systems/Account/services/account.ts
Lines 66 to 70 in 4fe865a
static async getAccounts() { | |
return db.transaction('r', db.accounts, async () => { | |
return db.accounts.toCollection().sortBy('name'); | |
}); | |
} |
you'll need to use toArray()
instead of toCollection()
in order to run .sort()
on it (and that sortBy('name')
can be removed).
I think this approach might work:
static async getAccounts() {
return db.transaction('r', db.accounts, async () => {
const accounts = await db.accounts.toArray();
return accounts.sort((a, b) => a.name.localeCompare(b.name, undefined, { numeric: true, sensitivity: 'base' }));
});
}
}); | ||
|
||
accounts = accounts.sort((a, b) => collator.compare(a.name, b.name)); | ||
|
||
return ( |
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.
also, could you add a short and clear changelog for this fix?
just run pnpm changeset
and follow the instruction (it looks like a patch fix)
When you have accounts
Account 1
... all the way toAccount 15
, the list ends up looking like this:etc.
This change makes the accounts list look like this: