-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add default sort to KTable
#824
Conversation
Update: I have added the tests for the
As you can see the |
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 :) |
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:
The purpose of
First thanks for all the docs updates. And I appreciated this particular example. This behavior seems useful to me, thanks!
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? |
Thanks a lot, @MisRob, for the response.
I see, but I would like to argue that the presence of such a prop like The introduction of the I am happy to be corrected if my understanding of these functionalities is at fault somewhere! Thanks for pointing the same out!
Yes. I initially wrote a basic test suite for both the principal components ( |
Hey @EshaanAgg, Thank you for working on this issue :)
Talking about the presence of an anti-pattern, in a case where |
Thanks @BabyElias! As you stated here, I had missed entirely the functionality of the Do you have any pointers on designing the API for this component going forward? Some queries I had for the same include:
|
@EshaanAgg, No worries - the lack of documentation for this mode can so lead to the confusion. About the queries,
Your opinions on this @MisRob ? |
Hi @BabyElias and @EshaanAgg, thanks both!
I think that'd help. We can definitely do smaller changes to improve clarity now.
Yes, issue here learningequality/kolibri#12705 for reference.
I think that would make sense. |
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. |
@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. |
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 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). |
@EshaanAgg |
Also, for the documentation -
|
Regarding @BabyElias:
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 :))! |
Definitely, thanks for following-up :)! |
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.
Yes! It turns out that our approach was a bit wrong. Let me explain. When we emit the The way that I think we should handle this is by introducing two separate handlers.
What do you think about this proposed API change? |
@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 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. |
Lovely, thank you @EshaanAgg. |
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 |
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? |
Yes, the tests are all fixed now I guess- so the documentation review part remains. |
Oh I didn't notice that, that's good news. Alright, I will read it in detail soon and confirm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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. |
Co-authored-by: Michaela Robosova <[email protected]>
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 :) |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @EshaanAgg, I have finished full code review. Nothing blocking.
/** | ||
* Disables the default sorting when sortable is true. Facilitates integration with externally sorted data. | ||
*/ | ||
disableDefaultSorting: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for me we will need to mention disableBuiltinSorting
-> disableBuiltinSorting
renaming as a breaking change in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, definitely. Thank you
Co-authored-by: Michaela Robosova <[email protected]>
:)
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 :) |
@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! |
Apart from the |
Merging, yey :) |
Description
Adds default sorting functionality to the
KTable
componentIssue addressed
Fixes #794
Changelog
Description: Adds default sorting functionality feature
Products impact: new API
Addresses: #794
Components: KTable
Breaking: no
Impacts a11y: no
Guidance: -
Description: Adds requirement for
columnId
attribute in allheader
objectsProducts impact: updated API
Addresses: #794
Components: KTable
Breaking: yes
Impacts a11y: no
Guidance: Add a unique column identifier
columnId
to allheader
objectsDescription: Renames
disableDefaultSorting
prop todisableBuiltinSorting
Products impact: updated API
Addresses: #794
Components: KTable
Breaking: yes
Impacts a11y: no
Guidance: Rename all occurrence of
disableDefaultSorting
Steps to test
Test out the component in the playground!
Implementation notes
At a high level, how did you implement this?
disableDefaultSorting
prop, as now we can use thedeafultSort
attribute to convey whether we want to sort the rows by default. I also removed the logic regarding emitting thechange sort
event as the same felt to be moot now.useLocalSort
(which sorted the rows by their complete values) but updated the API to allow developers to choose thecolumnId
they want to sort on.deafultSort
propKTable
componentdefaultSort
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
Reviewer guidance