Add default sort to KTable

[EshaanAgg]

Description

Adds default sorting functionality to the KTable component

Issue addressed

Fixes #794

Changelog

• Description: Added default sorting functionality to KTable
• Products impact: updated API
• Addresses: Fixes #794
• Components: KTable
• Breaking: no
• Impacts a11y: np
• Guidance: Added a new columnId field to all the headers, reworked the sorting handling, added more tests, and updated the component documentation

Steps to test

Test out the component in the playground!

Implementation notes

At a high level, how did you implement this?

• I have removed the disableDefaultSorting prop, as now we can use the deafultSort attribute to convey whether we want to sort the rows by default. I also removed the logic regarding emitting the change sort event as the same felt to be moot now.
• I have reworked the sorting logic not just to take a boolean of useLocalSort (which sorted the rows by their complete values) but updated the API to allow developers to choose the columnId they want to sort on.
• Better test coverage of the sorting composable with default sorting
• Added exhaustive prop validation for the new deafultSort prop
• Added more tests to the KTable component
• I added an example related to defaultSort on the documentation pages as well.

Please let me know if you see any possible regressions due to my implementation!

Does this introduce any tech-debt items?

No

Testing checklist

• Contributor has fully tested the PR manually
• Critical and brittle code paths are covered by unit tests
• The change is described in the changelog section above

Reviewer guidance

• Is the code clean and well-commented?
• Are there tests for this change?
• Are all UI components LTR and RTL compliant (if applicable)?
• Add other things to check for here

[EshaanAgg]

Update: I have added the tests for the defaultSort prop to the KTable suite, but there seems to be some error in the reactive handling that I can't seem to figure out. The outline of the approach that I am trying to follow is like:

• Use the defaultSort and headers props to get the column index based on which I should be performing the sorting.
• Supply the same to the useSorting hook.

As you can see the useSorting tests pass, and thus, we can be sure that the hook is handling the data as expected. There probably is some error in the way I am handling the reactive references to the props while calculating the useDefaultSorting reference in the setup function. I would greatly appreciate any help with the same!

[MisRob]

Flagging this PR for @BabyElias - as your time allows, if you'd like to, you're very much welcome to join the full or partial review :)

[MisRob]

Hi @EshaanAgg, thanks so much for your work on this :) I will dive into more detailed review as soon as we clarify a few things and debug the reactivity issues. So far, I read the PR description and the updated documentation. Couple of first higher-level thoughts:

I have removed the disableDefaultSorting prop, as now we can use the deafultSort attribute to convey whether we want to sort the rows by default. I also removed the logic regarding emitting the change sort event as the same felt to be moot now.

The purpose of disableDefaultSorting is to turn off internal sorting completely (this is for the case when the table receives sorted data from backend). The function of the defaultSort is to configure by which column should the table be sorted on an initial load. We may use it or not. There's some overlap, but I don't think that's the same, right? Seems names I chose are confusing here.

Docs "The defaultSort attribute can be used irrespective of the sortable attribute."

First thanks for all the docs updates. And I appreciated this particular example. This behavior seems useful to me, thanks!

there seems to be some error in the reactive handling that I can't seem to figure out

We'll be glad to help. I think I could understand a bit some parts. Just to be sure, you're referring to the test failure, right?

[EshaanAgg]

Thanks a lot, @MisRob, for the response.

The purpose of disableDefaultSorting is to turn off internal sorting completely (this is for the case when the table receives sorted data from backend). The function of the defaultSort is to configure by which column should the table be sorted on an initial load. We may use it or not. There's some overlap, but I don't think that's the same, right? Seems names I chose are confusing here.

I see, but I would like to argue that the presence of such a prop like useLocalSorting is somewhat of an antipattern. I say that because what if the user sets sortable to true and useLocalSorting to false? In such a case, according to the current implementation of the useSorting hook, we would just return the original rows without actually performing any sorting. At the same time, the UI headers would toggle (from asc -> desc -> default) and update their state, which is somewhat misleading (as no actual sorting of the data is being performed while the UI is conveying that the same does indeed occur).

The introduction of the defaultSort helps to clear up this ambiguity, which clearly states the precedence: always listen to the latest update by the user first, and then if no sorting is currently being applied, apply the default sorting by the column provided. This, in my opinion, is a much cleaner API. Having defaultSort to be empty effectively disables all the internal sorting in the table.

I am happy to be corrected if my understanding of these functionalities is at fault somewhere! Thanks for pointing the same out!

We'll be glad to help. I think I could understand a bit some parts. Just to be sure, you're referring to the test failure, right?

Yes. I initially wrote a basic test suite for both the principal components (useSorting and KTable) with the intended functionality associated with defaultSort and then tried to implement the changes to pass them. All the tests for useSorting have succeeded, while those for KTable have not. I can share a playground script and a brief description of what to look for that helps debug!

[BabyElias]

Hey @EshaanAgg, Thank you for working on this issue :)

I see, but I would like to argue that the presence of such a prop like useLocalSorting is somewhat of an antipattern. I say that because what if the user sets sortable to true and useLocalSorting to false? In such a case, according to the current implementation of the useSorting hook, we would just return the original rows without actually performing any sorting. At the same time, the UI headers would toggle (from asc -> desc -> default) and update their state, which is somewhat misleading (as no actual sorting of the data is being performed while the UI is conveying that the same does indeed occur).

Talking about the presence of an anti-pattern, in a case where sortable is set to true and disableDefaultSorting is set to true (i.e uselocalSorting set to false)- the implementation for this sorting experience has to be provided by the user and is independent of the useSorting hook. As you can see in the code here, if disableDefaultSorting is false - the table emits a changesort event(whose implementation can be defined by the user) and this event needs to return a sortOrder value for the UI headers to toggle. If the sortOrder value is not specified by the user - no toggling effect would occur. You can see an example of such a use-case in the playground page code here. Here, the changeSort event just simply reverses the columns upon being clicked and since no sortOrder value has been returned by the function - the headers are not toggled.

[EshaanAgg]

Thanks @BabyElias! As you stated here, I had missed entirely the functionality of the changesort event. Thanks again for clarifying this and resolving my query.

Do you have any pointers on designing the API for this component going forward? Some queries I had for the same include:

• Can we rename disableDefaultSorting to disableInternalSorting or anything else that communicates this indented behavior more clearly?
• We should probably document this behavior and the usage of this prop with examples in the documentation section!
• What should the expected behavior be if we set disableDefaultSorting to false and even provide a defaultSort attribute? Should the sorting occur or not in such a case?

[BabyElias]

@EshaanAgg, No worries - the lack of documentation for this mode can so lead to the confusion. About the queries,

• I think we can rename the prop to disableBuiltInSorting for more clarity, we already had discussed this as an option in a PR earlier (for reference, here)
• Yes, we do need to document this KTable mode - we are currently working on a real-application for this use-case in Kolibri. Once it is done, we will be able to identify the shortcomings of this mode, enhance the API and finally document it in a much better way.
• From the conversation about how we felt the need to introduce a defaultSort prop to the table - I believe this feature would only be needed for tables that have disableDefaultSorting set to false. For tables whose data is fetched from backend, configuring the external API to use the defaultSort option provided by KTable would rather be not needed as we can already request for data from backend in such a way that a column is sorted by default. In my opinion, KTable should just interact with changeSort in such a case and not have any interaction with the backend directly.

Your opinions on this @MisRob ?

[MisRob]

Hi @BabyElias and @EshaanAgg, thanks both!

I think we can rename the prop to disableBuiltInSorting for more clarity

I think that'd help. We can definitely do smaller changes to improve clarity now.

Yes, we do need to document this KTable mode - we are currently working on a real-application for this use-case in Kolibri. Once it is done, we will be able to identify the shortcomings of this mode, enhance the API and finally document it in a much better way.

Yes, issue here learningequality/kolibri#12705 for reference.

From the #772 about how we felt the need to introduce a defaultSort prop to the table - I believe this feature would only be needed for tables that have disableDefaultSorting set to false. For tables whose data is fetched from backend, configuring the external API to use the defaultSort option provided by KTable would rather be not needed as we can already request for data from backend in such a way that a column is sorted by default.

I think that would make sense.

[MisRob]

@EshaanAgg

Yes. I initially wrote a basic test suite for both the principal components (useSorting and KTable) with the intended functionality associated with defaultSort and then tried to implement the changes to pass them. All the tests for useSorting have succeeded, while those for KTable have not. I can share a playground script and a brief description of what to look for that helps debug!

I will have a look at this some time this week. I think I saw strange reactivity issues in another tests when using composables, so I wonder if it's related. If I don't have any success, we can try continuing debugging together or invite someone else from the team.

[MisRob]

@BabyElias @EshaanAgg For context, I hope for focused detailed work on table's public API specification at some point. Even though we touched on it a bit, it really wasn't the focus of the GSoC project. So hopefully we will fine-tune all these aspects. If you had some more feedback, now or in the future, even for breaking updates to the public interface, I just opened a tracking issue that should serve us well for discussions.

[EshaanAgg]

Thanks a lot for the reviews @MisRob and @BabyElias! Since we had a lot of discussion points and use cases to clear, I felt that it would be best first to update the documentation and then change the code. It would be a great help if you could review the public API along with the examples in the deploy preview and let me know if my understanding of the API, as well as the current implementation, produces the expected results or not.

I also had a query. If you would look at the Disable Builtin Sorting & Custom Sorting Logic section in the documentation, I was able to figure out how to implement the custom sorting via the changeSort event (thanks a lot to @BabyElias for sharing the playground script!), but I can't seem to figure out how to toggle the headers based on the return value of the function. Is there any existing functionality for the same, or the same remains to be implemented?

If all seems well, I can work on adding the tests (I thought it would be great to add all the documentation examples as integration tests to the component so that there are no regressions in the component, as I had introduced earlier).

[BabyElias]

@EshaanAgg
From the code that I see for disableBuiltInSort example on the playground page, I guess the issue is with not returning sortKey as part of the function or with reactivity. In the code that useSorting composable uses (responsible for sorting data on frontend when built in sorting is enabled), sortOrder and sortKey both are returned and as we can see in the part code responsible for icon toggle here - if sortOrder is not defined, then I don't think icon will be toggled as isColumnSortActive(index) will be false.
If even after this the code doesn't work, maybe we can try using ref with sortOrder and sortKey just to ensure it's not an issue with reactivity. Thanks!

[BabyElias]

Also, for the documentation -
Thank you so much for all the efforts in putting things there. The examples used are just perfect!
However, I am not sure about the description for Table with Default Sort section :

This is useful if you are getting unsorted data from the backend and want to display it in a sorted manner on load. The defaultSort attribute can be used irrespective of the sortable attribute (as sortable is used to configure whether the client has sorting capabilities or not).

• Maybe it would be better to replace this with : This is useful if the table needs to be sorted based on any column from the beginning itself (by default). We generally avoid using terms like client/backend in the documentation to reduce complexity and avoid confusion :)

To make use of defaultSort, please ensure that the disableBuiltinSorting attribute is not set to true as it will disable all sorting functionality.

• Disable all sorting functionality as in referring to the client-side sorting functionality here? I think just simply stating that defaultSort only works when disableBuiltInSorting set to false rather than specifying what actually happens then would also work.

[MisRob]

Regarding @BabyElias:

We generally avoid using terms like client/backend in the documentation to reduce complexity and avoid confusion :)

This was my request for the documentation - even though backend is one of the use-cases, the table could also receive data sorted from outside in any other way (e.g. external sort function that's still run on frontend). So basically the language would ideally be as simple and straightforward as possible and not making much assumption about how the component is used (the only exception I could think of would be a specific example, where we could say: For example, if receiving backend data...) I hope that's not way too nitpicky, it's based on my reading and some team feedback of the very first version of docs where we experimented a bit with this.

I will too read and reply to all the comments later in the week, but you two are on it so perhaps I won't have much work left by that time :))!

[MisRob]

@EshaanAgg

Since we had a lot of discussion points and use cases to clear, I felt that it would be best first to update the documentation and then change the code.

Definitely, thanks for following-up :)!

[EshaanAgg]

Maybe it would be better to replace this with : This is useful if the table needs to be sorted based on any column from the beginning itself (by default). We generally avoid using terms like client/backend in the documentation to reduce complexity and avoid confusion :)

I think just simply stating that defaultSort only works when disableBuiltInSorting set to false rather than specifying what actually happens then would also work.

Thanks for these suggestions! Completely agree that these changes make the documentation much easier to follow through and understand. Have implemented them in the latest push.

@EshaanAgg From the code that I see for disableBuiltInSort example on the playground page, I guess the issue is with not returning sortKey as part of the function or with reactivity. In the code that useSorting composable uses (responsible for sorting data on frontend when built in sorting is enabled), sortOrder and sortKey both are returned and as we can see in the part code responsible for icon toggle here - if sortOrder is not defined, then I don't think icon will be toggled as isColumnSortActive(index) will be false. If even after this the code doesn't work, maybe we can try using ref with sortOrder and sortKey just to ensure it's not an issue with reactivity. Thanks!

Yes! It turns out that our approach was a bit wrong. Let me explain. When we emit the changeSort event, we essentially notify the parent that he should sort the rows based on the click information, but we did not provide him with any means to do so! That's why in the playground script that you shared, you can change the value of the rows prop itself, which causes the complete component to rerender and thus is not the most optimal for the performance reason. The result of the emit event handler is never propagated to the child component, and thus it has no way of knowing the new updated sortKey and sortOrder.

The way that I think we should handle this is by introducing two separate handlers.

• We retain the changeSort event handler, so that the parent is notified when the headers are clicked and he is expected to provide a sorting functionality.
• We introduce a new prop called customSort, which the user can optionally define if he wants to implement a custom sorting logic. This function receives the current rows, the column index of the header, and the current sort key as arguments and is expected to return all three back, which can used by the KTable to update the state! I have updated the documentation to include an example.

What do you think about this proposed API change?

[MisRob]

We introduce a new prop called customSort, which the user can optionally define if he wants to implement a custom sorting logic.

@EshaanAgg Would it be fine to eventually move the discussion to a new proposal issue about the custom sort or #829? I'd be also fine doing it. I don't think the table has full support for it yet and there's more things to be decided about the public interface of custom sort functions. It seems it's not strictly necessary to make the defaultSort work well for the current use-case, right?

I think it's great to think about these optimizations, and I'm eager to continue iterating with both of you - just chiming in to help us organize this work so the scope can stay as narrow as possible in this PR for ease of review and discussion. Generally for new public APIs, more time is needed for the whole dev team for feedback, so proposal issues are good place.

[EshaanAgg]

Yes, @MisRob! Thanks for the input. I would add a comment on #829 specifying the idea that I had, as well as some examples of the same, and would limit the scope of this PR to just deafultSort.

[MisRob]

Lovely, thank you @EshaanAgg.

[MisRob]

Side note, even though I'm not planning to label #829 for the wider community for contribution, if this task would be interesting for one or both of you at any point in the future, that'd be a different story. I didn't really get an idea to mention it because it's larger time investment, and I feel we're already receiving so so much in KTable follow-up work :) However, there's no time pressure at all, and from this PR it seems there is lots of interest and fruitful api discussions already so at least wanted to mention it's open.

[MisRob]

Hi @EshaanAgg and @BabyElias, I think the next step now would be me confirming the documentation and then helping to debug the tests, is that right?

[BabyElias]

Yes, the tests are all fixed now I guess- so the documentation review part remains.

[MisRob]

Oh I didn't notice that, that's good news. Alright, I will read it in detail soon and confirm.

[MisRob]

@EshaanAgg I saw the docs and these updates are such an enrichment. As always, your work goes beyond all expectations :) KTable is shaping up so very nicely overall.

I used this opportunity to leave documentation suggestions. None of them are blocking by any means, and most of them you can commit right here in the UI.

Most importantly, regarding how KTable is supposed to work, I confirm this is exactly what I would expect at this stage. Thanks to both of you @BabyElias and @EshaanAgg clarifying this together.

@EshaanAgg let me know if there's something else I should do in this PR as a next step.

[EshaanAgg]

Thanks a lot, @MisRob, for the kind words and the detailed review! I will update the PR with the suggested changes concerning the style guide and request a review then! Since the tests are not passing and I was able to iron out the reactivity issues, I don't feel any special next steps are needed as a part of this PR.

[MisRob]

Lovely @EshaanAgg. Thank you. I will start more detailed code review next week. Seeing the documentation and tests I wouldn't expect any larger changes requested :)

[MisRob]

Side note, when you were dealing with reactivity and tests, by any chance have you observed something that would resemble these issues I got stuck on?

• Improve unit test coverage of KCardGrid and KCard #802
• Optimize KCardGrid's internal usage of watchers #801

[MisRob]

Thank you, @EshaanAgg, I have finished full code review. Nothing blocking.

[MisRob]

Just a note for me we will need to mention disableBuiltinSorting -> disableBuiltinSorting renaming as a breaking change in the changelog.

[EshaanAgg]

Another (not a strictly) breaking change would be the now mandatory requirement of columnId attribute in all the headers, if that may be good to mention in the changelog.

[MisRob]

Oh yes, definitely. Thank you

[EshaanAgg]

Thank you, @EshaanAgg, I have finished full code review. Nothing blocking.

:)

Side note, when you were dealing with reactivity and tests, by any chance have you observed something that would resemble these issues I got stuck on?

• Improve unit test coverage of KCardGrid and KCard #802
• Optimize KCardGrid's internal usage of watchers #801

I'm afraid not. The errors that I was facing were mostly due to the old versions of enzyme that we are using for testing, and me getting used to writing UI tests without using the Testing Library!

[MisRob]

I'm afraid not. The errors that I was facing were mostly due to the old versions of enzyme that we are using for testing, and me getting used to writing UI tests without using the Testing Library!

@EshaanAgg nevermind, thank you :)

[MisRob]

@EshaanAgg @BabyElias I am signing off soon for holidays till the end of this week, like many of us. I can take care of the changelog updates and then I think we could merge next week. Thanks for such a good chunk of work, again!