-
Notifications
You must be signed in to change notification settings - Fork 210
fix: trash page sorting and show more #4967
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
base: hotfixes
Are you sure you want to change the base?
fix: trash page sorting and show more #4967
Conversation
Patch release v2025.02.27
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.
This seems to be moving us in the right direction, but not clear how the backend is supporting these updates.
@@ -216,6 +216,10 @@ | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When was more an empty object?
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.
What I meant is that it's not null—not that it returns an empty object.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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
…