-
Notifications
You must be signed in to change notification settings - Fork 709
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
CoreTable: Do not render table data when data are loading #10993
Comments
Hello! I just want to let you know that I'm working on this issue. Hope to make a PR soon 🚀 |
Hi, @muditchoudhary, thank you! I'm assigning you (and also myself just to keep an eye in case you needed anything). If you haven't yet seen our contributing guidelines, please have a look. |
Hello @MisRob, I'm trying to replicate the issue on my local dev. Could you please tell me what section of Frontend architecture and Backend architecture in docs, I should focus on? Currently, I'm starting to understand the frontend architecture. Should I read the whole frontend and backend architecture section? Also what other section in the docs would be more helpful to read first? |
Hi @muditchoudhary, I think you don't need to study the whole documentation in detail, perhaps just skimming through it briefly so you know what sections are available. Not related to this issue, just an example - if you at some point needed to work with translated strings, you would hopefully remember you saw a section for that in docs and studied relevant information in detail at the point you would actually need it. Firstly, the most important thing would be to follow links in the contributing guidelines I mentioned to "Getting started" section. Then, when it comes to this particular issue, it may be helpful to checkout out the "Vue components" and the "Frontend code conventions" in the "Frontend architecture". Depending on your previous experience, you may also benefit from referring to Vue framework documentation as you study the relevant code in That said, if you're interested in long-term contributions or just want to learn more about how Kolibri work, you can choose any part of the documentation that's interesting for you and study it in more detail, of course. However this can also be done gradually as you work on issues. As you noticed, it's a huge resource and we certainly don't expect contributors to study it all. |
Thank you @MisRob for the detailed reply. I realized after looking at the code of the CoreTable.vue that I just need to understand some of the vue framework concepts to fix the issue. I have a good grasp of javascript and React and I see vue follow the same underlying concepts as React, so I think it won't be much difficult to understand CoreTable.vue and code some logic to hide group table. |
Hello, @MisRob I need some help understanding how What I TriedSo, I made 2 logic to hide the WRONG 2. Add v-if to tbody slot template to pass it's slot value only when dataLoading is false. File is: But I get an error that can't Iterate this.slots.tbody because it is null, which I understood. QuestionFor 1 logic I think it should work but it's not. The console.log statement also not getting printed. So, I don't quite understand how the CoreTable return statement renders core-table-container and core-table. Because I tried creating other element like p tag with hello world instead of tbody, but still I got the tobdy, how I don't know & want to understand. Also don't know why console.log not getting printed. I want your help in understanding what I have done wrong or missed. |
Hi @muditchoudhary, I'll have a look in the next couple of days. Would you mind sharing a link to your working branch in case I needed to have a look? |
Hi @MisRob this is the branch link: https://github.com/muditchoudhary/kolibri/tree/issue/10993-hide-table-when-data-are-loading I messed something, so it contains other contributors' commits too, I'll see and fix it before a PR. |
Thank you, @muditchoudhary. I am looking into it. Will reach out soon. |
Hi @muditchoudhary, so good news is that I think that your understanding of the core table code is great and you've got the solution (approach no.1) :)! It works on my local devserver. And this change you used for debugging
works for me too: So I'm wondering if your dev server is just not refreshing properly. If you try to delete all code of the CoreTable, and don't see any change in the page that uses it, than you can be certain that's the case. What commands do you use to run the dev server? I think you could open a PR if you'd like to. But if you'd like to keep contributing, let's resolve the dev server issues. |
Vue Devtools (https://devtools.vuejs.org/) is typically used for debugging Vue apps and has many useful features |
@muditchoudhary Please target |
Hi @MisRob Yeah I think too that was the issue of the dev server. Today I found out after debugging CoreTable line by line & replacing the loader with a p tag. I used I'll create a PR tomorrow. Thank you for taking the time to review my change and helping me :) |
@muditchoudhary Ah yes, can you try |
Yeah, I would love to explore the project more and contribute. Sure, I'll try to have my update regarding server issues by tomorrow. (If I understand and find out)
Thank you for sharing. I found it on Kolibri docs and already using it. |
Sure, Let me try it Update: Yeah it worked. I understood now. Thank you :) |
Hello @MisRob I was working on making a PR and When I run I got an error that I had activated the virtual env (with pyenv) I also tried to run this command to update python packages Here are the error logs of both commands in gist: https://gist.github.com/muditchoudhary/d2b14f1facb7057bee1fd72d58ed1b62 What I did before I get this error (it may not be the cause of errors but I'm still sharing for clarification)
|
@muditchoudhary Looking at your logs, it seems very similar to https://stackoverflow.com/a/65640477. Could you try if upgrading setuptools or any other suggestions there helps? |
Thank you! The solution description has worked for me. Can I also create a question regarding this in discussion and self-answer it? If anyone faces the same issue they may look it. |
I've made the PR. Please have a review #11238 |
Hello @MisRob I checked all good-first-issue and every issue is assigned. Do we have any issues in repo which is slightly harder and open for contributors? I would like to help. Or Which label I could use instead of good-first-issue? I'm a little comfortable with Kolibri's front-end code. I may help with this. |
Hi @muditchoudhary, thank you!
Of course, that'd be welcome. Here's the Q&A category. https://github.com/learningequality/kolibri/discussions/categories/q-a
You can ask for an assignment here #8532. Resolving it might take some time as there are many places to update and test. Meanwhile I will have a look if we can get ready some more issues or unassign other contributors from issues that seem to not have any progress for a long time. You can check out issues again from time to time to see if there's something new, and if not, feel free to let us know again. |
Awesome. I'll have a look from time to time on the issues until then I'll understand issue #8532. Thank you!! |
Great. You will need to comment on that issue for me to be able to assign you. |
Sure!! After understanding the issue a little bit. I'll ask you to assign me. |
Closed by #11238 |
Follow-up to #10944 (comment)
Observed behavior
In
CoreTable.vue
we show the circular loader when data are loading (that is whendataLoading
prop truthy)kolibri/kolibri/core/assets/src/views/CoreTable.vue
Lines 96 to 97 in 0e408ee
but there is no logic for hiding data in the table body when it is loading. That'd be fine under the assumption that whenever we're loading data, we can be certain that there is no data yet. Generally, this might not always be true in reactive environments and when using cached data.
Expected behavior
We'd like to make
CoreTable
more robust by hiding table data whendataLoading
prop is truthy.User-facing consequences
Displaying two states at a time, e.g. cached data together with a loader, would be confusing.
Acceptance criteria
dataLoading
prop is truthyCoreTable
is used are tested for regressionsThe text was updated successfully, but these errors were encountered: