Skip to content
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

Allow responding to annotations in a thread #4404

Merged
merged 112 commits into from
Apr 11, 2023
Merged

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Feb 10, 2023

This pull request adds threading to annotations. This allows users to respond to a specific annotation.
In order to add this functionality in the frontend, a major refactor of the frontend implementation of annotation is also contained in this pr.

I will first go over the functional changes from a user perspective. I will list the technical changes later.

Functional changes

First of all, the visuals of a user annotation have changed a bit:
old:
image
new:
image

The options to edit, delete or save an annotation for reuse are al moved to less intrusive dropdown button:
image

Clicking on the new answer input field opens up the existing larger form, that allows you to create a response on this annotation:
image

An open question inside a thread is clearly marked as such using an icon:
image

When you click the answer form, the question will be marked as in progress for one hour:
image

Adding an answer to a question will automatically resolve it:
image

An open question can also be resolved by clicking 'mark as answered', which will appear on each thread with an open question:
image

It is no longer possible to change the question status back to 'unanswered' using this interface. If desired I could add this option underneath dropdown.

Technical changes

Backend

The backend changes are rather minimal. An annotation now keeps track of it's thread_root_id.
If no thread root is defined an annotation is assumed to be the thread root.
Annotations with a thread root id are considered responses to that root annotation.

Only one level of threading is allowed. Thus a response to an annotation cannot be the thread root of another annotation. Validations and tests for this case are included.

Frontend

All annotation related code has been refactored to webcomponents and the state event system.

The webcomponents contain all code related to visualizing a component.

The state contains all data displayed on the page. The state is responsible for making requests to the backend, and notifying webcomponents if the data they are using has changed.

Splitting up these responsibilities should lead to more readable and more maintainable code.

As the amount of webcomponents increased a lot, I also decided to move some of them into more dedicated folders.

  • Tests were added
  • Documentation update can be found at dodona-edu/dodona-edu.github.io# (will do this after approval)

Closes #2827.

@jorg-vr jorg-vr added the feature New feature or request label Feb 10, 2023
@jorg-vr jorg-vr self-assigned this Feb 10, 2023
@jorg-vr jorg-vr marked this pull request as ready for review March 28, 2023 09:44
@jorg-vr jorg-vr requested a review from bmesuere March 28, 2023 09:44
@pdawyndt
Copy link
Contributor

pdawyndt commented Mar 28, 2023

Maybe "Reageer ..." as a translation of "Reply ..."? That's also somewhat the translation used in Google Docs ("Reactie toevoegen").

@chvp chvp self-requested a review March 28, 2023 12:27
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

The code looks good, I will do a final functional pass this afternoon.

Regarding the states, importing just the methods like now seems a bit strange because in the file using them you don't have a lot of context when you call them. If these would be objects a call like state.getXXX would be more clear. But as discussed, Potentially refactoring this (with or without redux) is something for a later PR.

Regarding my comments on the "meta" folder: after checking in detail, I agree that i18n, shadowless and watch can be called meta. The modal one is something different.

app/assets/javascripts/components/annotations/thread.ts Outdated Show resolved Hide resolved
@bmesuere bmesuere added deploy mestra Request a deployment on mestra and removed deploy mestra Request a deployment on mestra labels Mar 30, 2023
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

👍

app/assets/javascripts/state/UserAnnotations.ts Outdated Show resolved Hide resolved
@jorg-vr jorg-vr merged commit cc38019 into develop Apr 11, 2023
@jorg-vr jorg-vr deleted the feature/question-threading branch April 11, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy mestra Request a deployment on mestra feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread questions and annotations
5 participants