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

🐞 - Table sort icons in table header are reversed #9884

Open
hakimio opened this issue Dec 2, 2024 · 5 comments · May be fixed by #9904
Open

🐞 - Table sort icons in table header are reversed #9884

hakimio opened this issue Dec 2, 2024 · 5 comments · May be fixed by #9904

Comments

@hakimio
Copy link
Contributor

hakimio commented Dec 2, 2024

Reproduction url

https://taiga-ui.dev/components/table

Description

If you check th directive source code, you'll see that the sort icons are reversed:

return this.table?.direction === 1

direction === 1 means ascending order and -1 means descending order, but the code is the opposite way:

this.table?.direction === 1
    ? this.options.sortIcons.desc
    : this.options.sortIcons.asc;

Taiga UI version

4.17.0

Browser Used

Chrome

OS Used

Windows

@waterplea
Copy link
Collaborator

That would be breaking change :( It works right by default showing proper icons because the icons are also reversed. We can fix the default but if you overridden them in your app — this fix would break it for you. Or can this be considered a bugfix and not a breaking change?

@hakimio
Copy link
Contributor Author

hakimio commented Dec 3, 2024

It is a bug and should be treated as such. You can clearly see in the "Editable" and "With tuiSortBy directive" demos that the sorting icons are reversed.
Switching icons locally is a workaround, not a fix. The bug should be addressed in the library.

@waterplea
Copy link
Collaborator

Oh snap, you are correct, other libs have it the other way around. That's weird, I would expect chevron to show the direction of increasing value as I look through the table from top to bottom. Our proprietary icons look differently and it's clear what they mean :) Would you make a PR changing TH directive to switch the icons?

@hakimio
Copy link
Contributor Author

hakimio commented Dec 3, 2024

Do I only need to fix the TH directive itself or are there some tests that I need to update too? [Should there be some extra tests for this?]

@waterplea
Copy link
Collaborator

Just the TH, screenshots will fail and that would be expected.

@hakimio hakimio linked a pull request Dec 4, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants