fix: trash page sorting and show more#4967
fix: trash page sorting and show more#4967rtibbles merged 8 commits intolearningequality:hotfixesfrom
Conversation
Patch release v2025.02.27
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
This seems to be moving us in the right direction, but not clear how the backend is supporting these updates.
| // eslint-disable-next-line kolibri/vue-no-undefined-string-uses | ||
| return showMoreTranslator.$tr('showMore'); | ||
| }, | ||
| // "hasMore" returns true only if "more" is a nonempty plain object. |
There was a problem hiding this comment.
What I meant is that it's not null—not that it returns an empty object.
There was a problem hiding this comment.
Right, so I was wondering why this extra check was required, as it should either be null, or be an object with values.
There was a problem hiding this comment.
I usually write code with a future-proof mindset—anticipating that regardless of any backend changes, it will consistently return one of three possibilities: either an object with values, an empty object ( can be cases if we change something in code), or null.
I just like adding extra checks in the front-end to cover all conditions, making sure everything functions correctly even if the backend behavior changes in the future.
That extra check is a defensive coding measure to ensure the front-end logic remains robust.
I can remove it if you'd prefer.
There was a problem hiding this comment.
Please remove it - we have no reason to have this extra complicating logic.
Also the remangling of the more object into JSON and back seems to inflate the diff for no reason.
There was a problem hiding this comment.
contentcuration/contentcuration/frontend/channelEdit/vuex/contentNode/actions.js
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/channelEdit/views/trash/TrashModal.vue
Outdated
Show resolved
Hide resolved
|
Hi @GarvitSinghal47, is this ready for re-review? |
Yes just waiting for reply on this if confirmed i will remove it otherwise we can keep it as it is as well. |
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Code changes make sense, and manual testing checks out. I'm not seeing any evidence of regressions. This should be good to merge.
4 participants
Fix #4850
Summary
In this PR, I have updated the load children functionality to accept an ordering parameter, defaulting to lft. Additionally, the "show more" issue mentioned in #4851 is temporarily resolved by hiding the button if no new data is loaded after clicking it. I will investigate further and provide a comprehensive PR explaining why the network request sends data after all nodes have been loaded and resolve it in another pr seperately.
Screen-Recording.1.mp4
…
References
…
Reviewer guidance
…