-
Notifications
You must be signed in to change notification settings - Fork 27
Improve video table sorting #426
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: main
Are you sure you want to change the base?
Improve video table sorting #426
Conversation
Hello Berthold, |
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.
Thanks @berthob98,
I’ve left some comments. I also want to point out that suddenly removing or replacing the sorting is not an ideal solution.
The proper and sufficient fix is to only set: $table->maxsortkeys = 1;
This ensures only one sorting key is applied, which resolves the issue without introducing unnecessary complexity or sudden change of behavior.
$table->no_sorting('visibility'); // This column cannot be sortable because it does not mean anything to Opencast! | ||
$table->no_sorting('select'); | ||
$table->sortable(true, 'start_date', SORT_DESC); | ||
$table->text_sorting('title'); |
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.
Are we initiating the sort with title
instead of start_date
?
$table->no_sorting('select'); | ||
$table->sortable(true, 'start_date', SORT_DESC); | ||
$table->text_sorting('title'); | ||
$table->sortable(true, null, SORT_DESC); |
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.
We need to restore start_date
as the initial sorting column. Without it, the table does not follow the expected initial sort order.
$sortcolumns = $table->get_sort_columns(); | ||
|
||
// If we don't have a sortcolumn we sort by date desc. | ||
if (empty($sortcolumns)) { |
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 approach is not ideal. We are requesting the video list from Opencast sorted by start date, but the Moodle table is not being sorted accordingly. This logic should be removed to avoid inconsistent behavior.
Additionally, instead of using the hardcoded value 3, please use the SORT_DESC
constant for better readability and maintainability.
This PR adjusts the sorting of the videos table in the block overview page.
The current behaviour is, that the videos table is always sorted by the start_date, if a second column is picked in the UI the videos table gets multi sorted by the start_date and the picked column.
This PR changes the behaviour to fully sort the table by the newly picked column instead of multisorting.
If there won't be another Moodle 4.5 Release, I can rebase this PR for Moodle 5.0.