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

DataTable: Wrong alphabetical sorting for Norwegian (and others?) #4623

Open
rubjo opened this issue Oct 16, 2023 · 18 comments · May be fixed by #4628
Open

DataTable: Wrong alphabetical sorting for Norwegian (and others?) #4623

rubjo opened this issue Oct 16, 2023 · 18 comments · May be fixed by #4628
Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Milestone

Comments

@rubjo
Copy link
Contributor

rubjo commented Oct 16, 2023

Describe the bug

DataTable doesn't sort correctly with Nordic characters: "Å" is last in Norwegian, while DataTable sorts it immediately after "A".

localeCompare and Intl.Collator sort these correctly.

I suspect the same is the case for Danish, Swedish and possibly others.

How can I sort correctly by default – or how can I find documentation on how to do custom sorting?

Reproducer

https://stackblitz.com/edit/mczqps?file=src%2FApp.vue

PrimeVue version

3.36.0

Vue version

3.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

No response

Expected behavior

No response

@rubjo rubjo added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Oct 16, 2023
@mertsincan
Copy link
Member

Hi @rubjo,

Interesting! @FlipWarthog updated all sorting functions using Intl.Collator method
#4570

@FlipWarthog could you please check this issue?
Thanks a lot!

@mertsincan mertsincan added Status: Pending Review Issue or pull request is being reviewed by Core Team and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Oct 16, 2023
@FlipWarthog
Copy link
Contributor

@mertsincan Yep, you can assign this to me and I'll do some analysis.

@melloware
Copy link
Member

This should all work because its using

static localeComparator(locale) {
        return new Intl.Collator(locale, { numeric: true }).compare;
    }

It will use your currently set Locale for how to sort based on your language.

@melloware
Copy link
Member

You can test this easily in Chrome F12 Console by typing this...

new Intl.Collator('en-US', { numeric: true }).compare("123", "4ARD4");

image

Just switch for your locale instead of en-us and put in two String to compare with characters.

@rubjo
Copy link
Contributor Author

rubjo commented Oct 17, 2023

Thanks to @FlipWarthog for looking into this.

Meanwhile, I did a quick test on my Mac with Safari, Chrome, Firefox and Edge - which all report nb or nb-NO for Intl.NumberFormat().resolvedOptions().locale or navigator.languages[0].

In which case I guess undefined in the current return new Intl.Collator(undefined, { numeric: true }).compare; in ObjectUtils.js will work as expected.

But: For some reason, the browser I'm currently using (Arc), is the outlier here and reporting en-US. I might open an issue with them regarding this.

In any case, @FlipWarthog's change makes it possible for the app to be in charge of this, instead of leaving it to the browser.

Update: Arc just needed to have Norwegian added to the top of the list of languages in Settings, doh. I guess the other browsers automatically inferred this on first launch because of the OS locale.

@mertsincan mertsincan added this to the 4.0.0 milestone Dec 20, 2023
@mertsincan mertsincan added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add and removed Status: Pending Review Issue or pull request is being reviewed by Core Team labels Dec 20, 2023
@mertsincan
Copy link
Member

Thanks @FlipWarthog, I'll merge your PR in 4.0. I think we can use locale option instead of localeCode.

@FlipWarthog
Copy link
Contributor

@mertsincan No problem, let me know if you need me to rebase or make any updates, or if you just want to take over the PR. 👍

@goldenbull
Copy link

I have the same issue even with default en-US locale:

<template>
  <DataTable :value="items">
    <Column field="name" sortable header="name"/>
    <Column field="value" sortable header="value"/>
  </DataTable>
</template>

<script setup lang="ts">
import DataTable from 'primevue/datatable';
import Column from 'primevue/column';
import {ref} from "vue";

let items = ref(
    [
      {name: "@2833", value: "2833"},
      {name: "@a0", value: "a0"},
      {name: "@09091a", value: "09091a"},
      {name: "@11-", value: "11-"},
      {name: "@168a", value: "168a"},
    ]);
</script>

Here is the sorted table, which is not sorted correctly:

image

@goldenbull
Copy link

One more question: why numeric: true ?

here is a simple demo:

let ss = ["a11", "a16", "a090"];

console.log("numeric=true:" + ss.sort(new Intl.Collator('en', {numeric: true}).compare));
// output: numeric=true:a11,a16,a090
// this is WRONG because we are sorting strings, we expect '0' < '1'

console.log("numeric=false:" + ss.sort(new Intl.Collator('en', {numeric: false}).compare));
// output: numeric=false:a090,a11,a16
// this is the expected result

Seems we should use numeric: false when sorting column of string datatype.

@melloware
Copy link
Member

@goldenbull its because this comparer is ONLY used when the values are strings see:
https://github.com/primefaces/primevue/blob/10af0fff49e2704c501a68c7204a850378f9dfe0/components/lib/utils/ObjectUtils.js#L325C9-L325C112

So when comparing strings numeric:true because say you have these strings.

1,2,3,10,4,5

numeric:true will sort them properly.

1,2,3,4,5,10

numeric:false will sort them improperly

1,10,2,3,4,5

@goldenbull
Copy link

goldenbull commented Jan 31, 2024

@melloware I'm confused 😕 Well, my experience is mainly limited in C/C++/C#/Java, the "strong type" world. When a coder want to show a group of data in grid, it's the coder's duty to provide the data in correct datatype, string or numeric. The grid should sort the data according to it's datatype: if numeric, sort them as numeric; if string, sort them alphabetically.

Think about the above example, I have a string array:

let ss = ["a11", "a16", "a090"];

what is the correct (or expected) sorted result should it be?

seems that primevue provide an intelligent(automatic) way to deal with numeric data in string datatype, well, that's good, but on the other hand, the grid lost the ability to sort string data alphabetically. Maybe here should be some options for column? Something like

<Column field="code" header="Code" sortable numeric></Column>

@melloware
Copy link
Member

@goldenbull Correct.... but it should only do it if the column is a string.

    sort(value1, value2, order = 1, comparator, nullSortOrder = 1) {
        const result = this.compare(value1, value2, comparator, order);
        let finalSortOrder = order;

        // nullSortOrder == 1 means Excel like sort nulls at bottom
        if (this.isEmpty(value1) || this.isEmpty(value2)) {
            finalSortOrder = nullSortOrder === 1 ? order : nullSortOrder;
        }

        return finalSortOrder * result;
    },

    compare(value1, value2, comparator, order = 1) {
        let result = -1;
        const emptyValue1 = this.isEmpty(value1);
        const emptyValue2 = this.isEmpty(value2);

        if (emptyValue1 && emptyValue2) result = 0;
        else if (emptyValue1) result = order;
        else if (emptyValue2) result = -order;
        else if (typeof value1 === 'string' && typeof value2 === 'string') result = comparator(value1, value2);
        else result = value1 < value2 ? -1 : value1 > value2 ? 1 : 0;

        return result;
    },

It only uses the comparator if else if (typeof value1 === 'string' && typeof value2 === 'string') result = comparator(value1, value2); so that tells me it doesn't think its a string or there is another bug in PrimeVue not respecting numeric.

@goldenbull
Copy link

notice my above example, here are two strings: "a16" and "a090"
alphabetically, "a090" < "a16", because the second character, "0" < "1"
but when comparing using {numeric: true}, we got an amazing result: "a16" < "a090", because Javascript did some magic work, first, "a" == "a", then, numeric("16") < numeric("090"), which resulted in an unexpected order.

@goldenbull
Copy link

goldenbull commented Jan 31, 2024

here is the snapshot of Chrome console

image

Magic Javascript! 😄

@melloware
Copy link
Member

melloware commented Jan 31, 2024

Counterpoint:

image

true is correct order and false is incorrect order.

Original PrimeReact Issue: primefaces/primereact#4903

@goldenbull
Copy link

So how to sort ["a1", "a5", "a6", "a10"] lexicographically in primevue datatable? in some cases we need to treat the string data as is, just as we sort strings in C/C++/Java

@melloware
Copy link
Member

melloware commented Jan 31, 2024

Agreed I wonder if there is a better way to do this or maybe false should be the default?

Or maybe something set in the main PrimeVue config?

@goldenbull
Copy link

goldenbull commented Feb 1, 2024

how about this solution:

  1. set numeric: false as the default in compare initialization
  2. add an numeric option to Column class to alter the default compare initialization, like <Column field="code" header="Code" sortable numeric />

But this solution needs more code refactor and is error-prone when sorting multiple columns. Simpler solution is an extra option in main PrimeVue config and set numeric: false by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants