Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alfiedotwtf
Copy link

When you have accounts Account 1 ... all the way to Account 15, the list ends up looking like this:

  • Account 1
  • Account 10
  • Account 11
  • ...
  • Account 15
  • Account 2
  • Account 3

etc.

This change makes the accounts list look like this:

  • Account 1
  • Account 2
  • ...
  • Account 9
  • Account 10
  • Account 11
  • ...

Copy link
Member

@helciofranco helciofranco left a 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?

Comment on lines +35 to +40
const collator = new Intl.Collator(undefined, {
numeric: true,
sensitivity: "base"
});

accounts = accounts.sort((a, b) => collator.compare(a.name, b.name));
Copy link
Member

@helciofranco helciofranco Dec 9, 2024

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

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 (
Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants