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

[KTable]: Update Sorting Flow to Include "Unsorted" State #797

Closed
2 tasks
BabyElias opened this issue Oct 7, 2024 · 15 comments · Fixed by #803
Closed
2 tasks

[KTable]: Update Sorting Flow to Include "Unsorted" State #797

BabyElias opened this issue Oct 7, 2024 · 15 comments · Fixed by #803
Assignees
Labels
Component: KTable help wanted Open source contributors welcome P2 - normal Priority: Nice to have

Comments

@BabyElias
Copy link
Contributor

BabyElias commented Oct 7, 2024

🌱 Are you new to the codebase? Welcome! Please see the contributing guidelines.

Summary

Currently, once a user clicks on a header to sort a column in KTable, the sorting flow cycles between unsorted > ascending > descending > ascending > descending, with no way to return to the default unsorted state without refreshing the page. This issue aims to update the sorting behavior to allow users to return to the unsorted/default state after any column header has been clicked.

The Value Add

Adds flexibility in how users can interact with the table, especially when sorting is not desired or needs to be reset.

New Sorting Flow

The desired behavior after clicking a sortable column header should be:

  • unsorted > ascending > descending > unsorted > ascending > descending and so on.
  • This means that after a column has been sorted in first ascending, then descending order, clicking the header again should return it to the unsorted/default state before cycling back to ascending order.

Guidance

  • Implement an additional step in the sorting flow for resetting the column to the default unsorted state.
  • Note : This is only for cases where <KTable sortable> is used i.e KTable with enabled default sorting, which makes use of the useSorting composable as referenced here.

Acceptance Criteria

  • Clicking a sortable header cycles through the new sorting order: unsorted > ascending > descending > unsorted > ....
  • Unit tests are added or updated to verify the new sorting flow (if any)
@MisRob MisRob added Component: KTable help wanted Open source contributors welcome P2 - normal Priority: Nice to have labels Oct 8, 2024
@Sahil-Sinha-11
Copy link
Contributor

Hey @BabyElias @MisRob I would like to work on this issue. Please assign me this issue.

@MisRob
Copy link
Member

MisRob commented Oct 14, 2024

Hi @Sahil-Sinha-11, thank you for volunteering. I will assign you. Please take some time to understand the current KTable and this issue. You're welcome to ask questions here. Note that I will be out of office till the end of October so please mention @BabyElias or @AlexVelezLl during that period of time.

@Sahil-Sinha-11
Copy link
Contributor

https://github.com/user-attachments/assets/718d1dff-80bf-4c48-9428-69a707611e63
Hey @AlexVelezLl I added the unsorted order and it works like this , is it fine? Also How to implement the unit test part?

@MisRob
Copy link
Member

MisRob commented Oct 14, 2024

I don't see the new sorting flow as described in the issue in the video, @Sahil-Sinha-11 as you don't click the sort buttons there.

Regarding unit test, please familiarize yourself with the existing testing suite and use it as a guide.

@MisRob
Copy link
Member

MisRob commented Oct 15, 2024

I don't see the new sorting flow as described in the issue in the video, @Sahil-Sinha-11 as you don't click the sort buttons there.

I'm sorry @Sahil-Sinha-11, this was just a technical issue on my side - for some reason I saw time elapsing but nothing happening in the video. I tried to preview it again and it works now. Yes, this is expected behavior, thank you.

Here's the reference to unit tests https://github.com/learningequality/kolibri-design-system/tree/develop/lib/KTable/useSorting/__tests__, even though you may also consider starting a new spec file for KTable itself and test exactly what you show in video - clicking on the sort buttons.

@MisRob
Copy link
Member

MisRob commented Oct 15, 2024

@Sahil-Sinha-11 If that helps, you can find many more tests in other components, such as https://github.com/learningequality/kolibri-design-system/tree/develop/lib/tabs/tests, to serve as inspiration. We use Vue 2 + Jest + Vue Test Utils. Let us know if you had more questions.

@Sahil-Sinha-11
Copy link
Contributor

Sahil-Sinha-11 commented Oct 16, 2024

I'm sorry @Sahil-Sinha-11, this was just a technical issue on my side - for some reason I saw time elapsing but nothing happening in the video. I tried to preview it again and it works now. Yes, this is expected behavior, thank you.

Its fine and thanks for the guidance.

@Sahil-Sinha-11
Copy link
Contributor

Image
Hey @AlexVelezLl I added Test for the age section, sorting it back to default value, is this the right way?

@BabyElias
Copy link
Contributor Author

BabyElias commented Oct 16, 2024

@Sahil-Sinha-11 Yes, this is the right way to test the change introduced. Did you run the yarn run test command to see that all checks are passing?

@Sahil-Sinha-11
Copy link
Contributor

Image
@BabyElias It show this ... what should I do?

@bjester
Copy link
Member

bjester commented Oct 16, 2024

@Sahil-Sinha-11 You might try running the tests with the Jest CLI option --runInBand, as it might shed more light on the errors.

You should be able to do so with yarn run test --runInBand

@Sahil-Sinha-11
Copy link
Contributor

@BabyElias The error encountered is not in the file I made changes in I made changes in[i.e. i made changes in Ktable but the test fails for kTabLists] should I proceed with a PR?

@BabyElias
Copy link
Contributor Author

BabyElias commented Oct 19, 2024

@Sahil-Sinha-11 You can go ahead and raise the PR.
As for the failed test, I believe it's not even a component issue. I hope you are working based on the develop branch and have taken a recent pull for the file changes there. In the case, this can be a memory management/configuration issue within your own system, since the error indicates that the Jest suite could not complete execution, and that's leading to the failed test.
We'll look at it again if the issue persists in the raised PR tests. Thanks!

@Sahil-Sinha-11
Copy link
Contributor

@BabyElias I raised a PR #803 .

@AlexVelezLl
Copy link
Member

Closed by #803

@AlexVelezLl AlexVelezLl linked a pull request Jan 7, 2025 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: KTable help wanted Open source contributors welcome P2 - normal Priority: Nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants