-
Notifications
You must be signed in to change notification settings - Fork 26
feat: show loading screen in task area only #256
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
Conversation
93ca8a5 to
a2f2166
Compare
julien-nc
left a comment
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 looks great! A few things to adjust and we're good to go.
The task list item is not updated when the task finishes. Maybe we should fire an event in the event bus after pollTask so the TaskList component can update its internal task object status.
emit('task-updated', task)
which would be caught in TaskList and call a method like:
updateTask(task) {
const internalTask = this.tasks.find(t => t.id === task.id)
if (internalTask) {
internalTask.status = task.status
}
}
Is Another alternative could be instead of declaring the tasks list in the |
It's a lot of chaining props to carry the value from assistant.js to TaskList. I would be in favor of using the event bus. When using this function: |
bcdb3d1 to
08c1af1
Compare
done in 08c1af1, but using |
julien-nc
left a comment
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.
Works great. @janepie Could you give it a try as well?
@edward-ly Let's wait for Jana's feedback to merge this if you don't mind.
|
Nice one! |
|
@edward-ly You can rebase your branch on main where part of the task type switch issue was fixed (by #262). |
08c1af1 to
12ecdad
Compare
Good catch, should be fixed in 12ecdad now, mind testing one last time? @janepie @julien-nc |
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.
The issue with switching task types while a task is "loading" is fixed when opening the assistant with the menu entry but not when opening it from a notification. See inline comment.
12ecdad to
1005bd3
Compare
Not necessarily the red color, but I do see the button being in the wrong state too, latest commit should fix it. @janepie feel free to merge this if everything looks good to you |
…tion property Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
… click, toggle notification request instead Signed-off-by: Edward Ly <[email protected]>
…ng screen depending on task status Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
b7ec048 to
d3110c3
Compare
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
Signed-off-by: Edward Ly <[email protected]>
d3110c3 to
d03bead
Compare
Ah I see, I fixed the case for when a task from the task list is loaded, but not when switching task types or creating a new task, should be fixed now. |
janepie
left a comment
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.
Looks good, really nice work!
* Adjust `get notified` string. * Check for changed icon rather than message. Changes caused by nextcloud/assistant#256 Signed-off-by: Max <[email protected]>
* Adjust `get notified` string. * Check for changed icon rather than message. Changes caused by nextcloud/assistant#256 Signed-off-by: Max <[email protected]>
* Adjust `get notified` string. * Check for changed icon rather than message. Changes caused by nextcloud/assistant#256 Signed-off-by: Max <[email protected]>


Fixes #245. In addition, the task list is automatically updated when a new task is submitted or a running task is canceled, with the submitted task also being automatically selected from the list.