kolibri-design-system icon indicating copy to clipboard operation
kolibri-design-system copied to clipboard

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

Open BabyElias opened this issue 1 year ago • 1 comments

🌱 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)

BabyElias avatar Oct 07 '24 02:10 BabyElias

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

Sahil-Sinha-11 avatar Oct 11 '24 18:10 Sahil-Sinha-11

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.

MisRob avatar Oct 14 '24 09:10 MisRob

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?

Sahil-Sinha-11 avatar Oct 14 '24 19:10 Sahil-Sinha-11

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 avatar Oct 14 '24 19:10 MisRob

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 avatar Oct 15 '24 07:10 MisRob

@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.

MisRob avatar Oct 15 '24 07:10 MisRob

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 avatar Oct 16 '24 13:10 Sahil-Sinha-11

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

Sahil-Sinha-11 avatar Oct 16 '24 16:10 Sahil-Sinha-11

@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?

BabyElias avatar Oct 16 '24 17:10 BabyElias

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

Sahil-Sinha-11 avatar Oct 16 '24 19:10 Sahil-Sinha-11

@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

bjester avatar Oct 16 '24 20:10 bjester

@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?

Sahil-Sinha-11 avatar Oct 19 '24 04:10 Sahil-Sinha-11

@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!

BabyElias avatar Oct 19 '24 05:10 BabyElias

@BabyElias I raised a PR #803 .

Sahil-Sinha-11 avatar Oct 19 '24 08:10 Sahil-Sinha-11

Closed by #803

AlexVelezLl avatar Oct 23 '24 20:10 AlexVelezLl