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 easy re-use of annotations #3600

Merged
merged 106 commits into from
Sep 19, 2022
Merged

Allow easy re-use of annotations #3600

merged 106 commits into from
Sep 19, 2022

Conversation

chvp
Copy link
Member

@chvp chvp commented May 6, 2022

This pull request adds the re-use of annotations. Annotations are saved separately for re-use, with an extra added title.

Visuals:

When creating a new annotation, you now have more options
image

You can search for an existing comment
image

This automatically loads the saved comment text, but you can still edit it as needed
image

You can save each annotation for reuse, this automatically suggest the first five words as a keyword title
image

There is info on the annotation showing it is created using a saved annotation
image

A side bar shows al saved annotations for this exercise with some basic info
image

You can edit a saved annotation for later reuse from this sidebar
image

You can also save an existing annotation, there is a button for this on non saved annotations
image

Which opens a similar popup
image

There is also a page on which you can manage all your existing saved comments
image

Technical

As the management of saved annotations all happens on the evaluation page, I chose to limit the api to a json rest api. (No html or js delivered except the index page)
All visualization in the frontend happens using webcomponents.
As displaying the saved annotations and manipulating them happens in different components loaded separately on the page, I created an external reactive state which triggers component rerenders when the underlying model changes.
I tried to write this state system in such a way that it would be relevant for any restfull resource.

I also created a generic modal webcomponent mixin to fix working with modals as webcomponents.

For the listing I created a generic pagination component that mimics the behavior of will_paginate. I added pagination meta data to the request headers.

I also had to edit the webpack config for this, because even with the commons js pack, PubSub was loaded multiple times, resulting in publishers publishing to an object without subscribers. The single runtime optimization solves this.

This newly created pack is negligible in size (1.55 kb before minimization)
image

I have also removed https://github.com/dpogue/rails_server_timings
It is no longer actively maintained and causes the following bug: dpogue/rails_server_timings#6
It was already disabled in all config files except the test config (And I do not see its added value in an automatic test)

  • Tests were added

The plan is to first allow this in a (very) closed beta, so I will wait with writing documentation until the results of that closed beta are clear.

Closes #2825.

@chvp chvp added the feature New feature or request label May 6, 2022
@chvp chvp force-pushed the feature/annotation-reuse branch 3 times, most recently from 8c838a6 to 755e5dc Compare May 11, 2022 08:18
@chvp chvp force-pushed the feature/annotation-reuse branch 2 times, most recently from c2276d6 to ce9fe83 Compare May 11, 2022 08:47
@chvp chvp force-pushed the feature/annotation-reuse branch 2 times, most recently from dc86204 to cc27b0b Compare May 13, 2022 08:24
@jorg-vr jorg-vr self-assigned this Jun 13, 2022
@lgtm-com
Copy link

lgtm-com bot commented Jun 16, 2022

This pull request introduces 1 alert when merging f8aad09 into f75a36c - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request introduces 5 alerts when merging 361bc86 into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request introduces 5 alerts when merging 0118b8f into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 17, 2022

This pull request introduces 5 alerts when merging a115ce9 into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 20, 2022

This pull request introduces 5 alerts when merging a3a8ead into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 20, 2022

This pull request introduces 5 alerts when merging 9a6f3b3 into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 20, 2022

This pull request introduces 5 alerts when merging dde7f20 into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined
  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jun 20, 2022

This pull request introduces 4 alerts when merging bc919df into 7f602f7 - view on LGTM.com

new alerts:

  • 4 for Property access on null or undefined

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging 3150311 into be1bf3e - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@jorg-vr
Copy link
Contributor

jorg-vr commented Sep 13, 2022

@bmesuere I think I handled all your comments.

I'll add some more tests later, but would prefer to have your review first. (So I don't need to change the tests multiple times because of functionality changes)

@jorg-vr jorg-vr marked this pull request as ready for review September 13, 2022 09:00
@jorg-vr jorg-vr requested a review from bmesuere September 13, 2022 09:00
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Sep 15, 2022
@bmesuere bmesuere temporarily deployed to mestra September 15, 2022 13:15 Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Sep 15, 2022
app/assets/javascripts/saved_annotation_beta.ts Outdated Show resolved Hide resolved
app/policies/saved_annotation_policy.rb Outdated Show resolved Hide resolved
app/assets/javascripts/components/pagination.ts Outdated Show resolved Hide resolved
app/assets/javascripts/components/pagination.ts Outdated Show resolved Hide resolved
app/assets/javascripts/i18n/translations.js Outdated Show resolved Hide resolved
@chvp
Copy link
Member Author

chvp commented Sep 15, 2022

It seems weird to me that one can save an annotation when re-using a saved annotation. (During annotation creation.)

@jorg-vr
Copy link
Contributor

jorg-vr commented Sep 15, 2022

@chvp i was unsure about this. Potential usecase (why I allowed it) is if you want to start from a saved annotation, edit it and save it again as a variant

@chvp
Copy link
Member Author

chvp commented Sep 15, 2022

Makes sense. Definitely fine to leave in for the closed beta.

@jorg-vr jorg-vr merged commit 0b6c1de into develop Sep 19, 2022
@jorg-vr jorg-vr deleted the feature/annotation-reuse branch September 19, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow re-use of annotations
4 participants