Skip to content

Edit task page rewritten to Vue#789

Open
SimonAdamek wants to merge 13 commits intomrlvsb:masterfrom
SimonAdamek:edittask-vue
Open

Edit task page rewritten to Vue#789
SimonAdamek wants to merge 13 commits intomrlvsb:masterfrom
SimonAdamek:edittask-vue

Conversation

@SimonAdamek
Copy link
Contributor

This is a rewrite of Edit task page into Vue.

Old svelte version is still present and can be accessed by modifying URL
http://127.0.0.1:8000/task/edit/8/ -> http://127.0.0.1:8000/#/task/edit/8/

Main changes:

  • load page using standard Django urls.py and templates
  • add useSvelteStoreInVue.ts to connect Vue components to the existing Svelte store
  • rewrite the entire editor component (the previous version was present but malfunctioning)

The rest is essentially a 1:1 rewrite of the old components, with minor changes to make them work in Vue and make it little more clean

I tested editing and creating tasks, file management, tests management, and other related functionality. Everything appears to work the same way as before.

@SimonAdamek SimonAdamek force-pushed the edittask-vue branch 4 times, most recently from 959f31f to 35ae101 Compare December 7, 2025 14:29
Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thank you for working on this, it will be great once we can move all of this to Vue! :) This is a pretty big change, and I'd like to try to land it in smaller pieces. Since you found a way to have both the Vue and Svelte versions running side-by-side, do you think that we could land the Vue rewrites in smaller chunks, while keeping Svelte as the default, and then do the switch and remove Svelte once everything has been migrated? It's quite hard to review a 2k line diff PR.

@SimonAdamek SimonAdamek force-pushed the edittask-vue branch 3 times, most recently from fa5cc4d to cb413f0 Compare December 8, 2025 17:59
@SimonAdamek
Copy link
Contributor Author

Hi, thank you for working on this, it will be great once we can move all of this to Vue! :) This is a pretty big change, and I'd like to try to land it in smaller pieces. Since you found a way to have both the Vue and Svelte versions running side-by-side, do you think that we could land the Vue rewrites in smaller chunks, while keeping Svelte as the default, and then do the switch and remove Svelte once everything has been migrated? It's quite hard to review a 2k line diff PR.

If you mean to take a Vue component and use it in Svelte, that is probably a problem. Theoretically, it should work, but according to ChatGPT, there is a problem with v-model, which cannot be used. It would require modifying both the Vue and the original Svelte code (bind cannot be used). So basically only Tests.vue and FileManager.vue could be tested this way.

I know that 2000 lines is a huge PR. But apart from the editor, I basically just added types/interfaces to the old JS code and added a wrapper for the Svelte store. For easier testing I did not change the functionality anywhere.

In the templates I didn't change anything, just changed the Svelte directives to Vue (if, for, function calls, etc.) here.

@Kobzol
Copy link
Collaborator

Kobzol commented Dec 8, 2025

I thought that since you mentioned that both the Svelte and Vue versions work, that we wouldn't need to use Vue components in Svelte. Just land the new Vue version piece-by-piece until everything is ported. But if you think it would be too complicated, I'll try to review this PR..

@Kobzol
Copy link
Collaborator

Kobzol commented Dec 8, 2025

Could you please open a separate PR for d6ee3d1? Thanks!

@Kobzol
Copy link
Collaborator

Kobzol commented Dec 30, 2025

I tested this locally, and found some bugs:

  • Using Ctrl + Enter (for autocompletion) in the config.yml file doesn't seem to work.
  • The Set assigned date, Set relative deadline and Set deadline buttons don't seem to always work correctly. For example, if I have two tasks, one has a deadline, the other doesn't, then clicking on "Set deadline to all assigned classes" doesn't work. Then if I click on "Set assigned date to all visible classes", it sets the deadline instead. Maybe this is a bug caused by the :key="idx" problem?
  • When clicking on "Assign new task" on the teacher homepage (with the list of classes), the current semester is not prefilled correctly in the path to the task to be created.
  • Clickin on the pencil (Edit task) button from the task page (e.g. http://127.0.0.1:8000/task/7054/BER0134/) redirects to the Svelte version of the page, rather than the Vue version of the page.
  • When clicking on the Duplicate this task button, the page breaks and there is an error in the console:
    image
  • When clicking on the delete task button (red bin, seems like it has no title in Svelte nor Vue), the page breaks and there is an error in the console:
    image

Otherwise it seems to work really well. Great job! I would actually prefer removing the Svelte version completely in this PR, so that we don't have two parallel versions being potentially used. It will motivate us to fix any issues in the Vue version faster, and actually start using is in production.

Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some review comments.

@SimonAdamek
Copy link
Contributor Author

So i finally managed to get the second version of the Editor.vue work. I also try to solve all mentioned bugs.

I only can't reproduce the one with assign date buttons. Probably i fix it with :key="idx". Can you look if it works now?

@Kobzol
Copy link
Collaborator

Kobzol commented Jan 23, 2026

(I couldn't test it without rebasing locally, so I rebased it for you and force-pushed here. git fetch <remote> && git reset --hard <remote>/edittask-vue to update to it.)

@Kobzol
Copy link
Collaborator

Kobzol commented Jan 23, 2026

Oh, and the backend for comments has changed in the meantime, because the student page TaskDetail has also switched to Vue, and the comment endpoints were migrated to Django Ninja.

Well, even though I kind of resigned to doing things properly via small PRs, this PR became a monstrosity.. 😆 But if you can get it working after the rebase with the new backend API, and the bugs I found earlier are fixed, I'm willing to merge it at once and then fix bugs once we encounter them in the wild.

@SimonAdamek
Copy link
Contributor Author

I agree that it's very large. The problem is that it's simply a set of files that depend on each other. I think it would still be a problem to migrate it in parts....

So I hope it should work now..

@Kobzol
Copy link
Collaborator

Kobzol commented Jan 23, 2026

Yeah it would have to be done incrementally, while the old Svelte page would still work. I will take a look at the functionality, just wanted to note that if we replace the whole Edit task page in one PR, we should also nuke the Svelte version at once.

Copy link
Collaborator

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some review comments.

<ul v-if="filtered.length && focused">
<li
v-for="(item, i) in filtered"
:key="i"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ok, because the index i will be reused for different items once filtered changes. Let's remove the key altogether, since there is no inner state of the rendered component.

@Kobzol
Copy link
Collaborator

Kobzol commented Jan 24, 2026

Did another UX test, here are some notes:

  • Copy hard deadline to all assigned classes doesn't always work. Clicked on the button next to hard deadline here and nothing happened:
    • image
    • It seems to work like 90% of the time 😆
  • Similar to above, sometimes when I check/uncheck the hard deadline and save the page, the change disappears after reload.
  • Setting max points to an empty text field breaks the backend: ValueError: Field 'max_points' expected a number but got ''.. Probably there was some empty string -> None/NULL conversion happening somewhere previously?
  • The "Duplicate task" button still behaves a bit weirdly:
    • image
    • I use a symlink for the tasks directory, maybe that is causing some issues? 😅
  • After a task is deleted, there is an error in the console:
    • image
    • Maybe it would be easier to just redirect to / after deleting?

Could you please also completely remove the Svelte version of EditTask (and the Svelte components that were only used by it) in this PR? Just to check if we do not depend on anything from Svelte anymore on this page.

I think that the issues with the classes is due to the way you implement reactivity in the EditTask.vue component. Vue's watch watches the refs in a shallow way. So if you do watch(task, ...) and then change task.value.foo..., it won't be seen by the watch! Also if showAllClasses changes, then the watch won't be recomputed. You can test it here. You can make the watch deep, but that has some performance implications. And I'm kinda unconvinced that watch is even needed here. Can't shownClasses just be a computed property that depends on task? In general, watch is IMO an anti-pattern, as long as there is a way to do the same logic without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants