switch beetween ASC, DESC, NO filter

[Ennoriel]

Hello,

It's me again.

When clicking on a column, if the column is sortable, it is sorted ASC, then DESC and there is no way to remove the sorting criteria. I would maybe suggest to have a prop to either switching from ASC to DESC or from ASC to DESC to NO filter. Also, usually, when a column if sortable, there is a two-arrow icon indicating that the column is sortable.

I haven't tried but it is theoretically possible to override this behavior with the exposed clickCol event and sortorder / sortby props.

If you find this relevant, I can submit a PR.

[dasDaniel]

That sounds useful.

I'm thinking this part
would use an array for sort order and the default would be the first value, so [1, -1] would be the current functionality and setting to [1, -1, 0] would be the functionality you describe. I think for ease of use, order consts like ORDER_ASC could be exported through module script since they are more intuitive than using number values.

  const updateSortOrder = colKey => {
    if (colKey === sortBy) {
      sortOrder = sortOrder === 1 ? -1 : 1;
    } else {
      sortOrder = 1;
    }
  };

the one thing I'm not sure on is whether a setting of 0 would just propagate to the filter or if the updateSortOrder method should treat 0 as sortBy = "" thereby clearing the sort setting (so [1, 0 ,-1] could never reach -1 value)

[Ennoriel]

Your idea of a sortOrders prop is great ! That would also allow anyone to order ASC or DESC first.

I would go with something like this:

export let sortOrders = [1, -1]

const updateSortOrder = colKey => {
    if (colKey === sortBy) {
        sortOrder = sortOrders[(sortOrders.findIndex(o =>  o === sortOrder) + 1) % sortOrders.length];
    } else {
        sortOrder = 1;
        // we could even have somting like: sortOrders[0];
    }
};

Also, I prefer having a method that returns a value (setOrder) to have tests ;)

Jest tests

test('updateSortOrder', () => {
    // const updateSortOrder = (colKey, sortBy, sortOrder, sortOrders)

    expect(updateSortOrder('name', 'year', undefined, [1, -1])).toStrictEqual(1);
    expect(updateSortOrder('name', 'year', undefined, [1, -1, 0])).toStrictEqual(1);

    expect(updateSortOrder('name', 'name', undefined, [1, -1])).toStrictEqual(1);
    expect(updateSortOrder('name', 'name', 1, [1, -1])).toStrictEqual(-1);
    expect(updateSortOrder('name', 'name', -1, [1, -1])).toStrictEqual(1);

    expect(updateSortOrder('name', 'name', undefined, [1, -1, 0])).toStrictEqual(1);
    expect(updateSortOrder('name', 'name', 1, [1, -1, 0])).toStrictEqual(-1);
    expect(updateSortOrder('name', 'name', -1, [1, -1, 0])).toStrictEqual(0);
    expect(updateSortOrder('name', 'name', 0, [1, -1, 0])).toStrictEqual(1);
})

[dasDaniel]

agreed, a functional approach is better. And having handleClickCol pass the variables could make easier to add later the ability to configure sort order per-column.

[Ennoriel]

I came up with a question: I know that it would be a breaking changes, but, does it make sense to have ASC/DESC by default without the opportunity to unset a filter?
If you want me to submit a PR, let me know.

[dasDaniel]

Unless there's a compelling reason I would keep default as ASC/DESC without ability to unset.
I don't want to introduce breaking changes, and default sorting should be supplied (and the docs should probably make that clear)

a PR is welcome, thanks

[dasDaniel]

Thanks
Merged, build and pushed to npm under v0.3.3