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

Add annotations overview page & improve Cypress commands #668

Merged
merged 77 commits into from
Aug 13, 2024

Conversation

Splines
Copy link
Member

@Splines Splines commented Aug 2, 2024

This resolves #658

This adds a new page where users can view all of their annotations across all lectures. The list items can be clicked to jump to the timestamp at the annotation of the respective video.

image

image

TODO

  • Color according to annotation
  • Button to open annotation in video -> no, instead just clickable list item
  • Pagination -> as annotations are grouped by lectures in this view, I don't think we need pagination. Even if the list becomes big, it's still manageable by just scrolling on the page a bit.
  • Categorize annotations in subsections for lectures
  • No annotations at all -> add link to blog post to see how to create them
  • Allow rendering of math content inside annotations -> had nothing to do here, worked automatically
  • Localization
  • Deal with annotations that were already posted as comments -> have no comment field anymore
  • Caching issue when accessing page with timestamp
  • Open annotation at timestamp (user shouldn't have to make another click to open the annotation)
  • Annotation overview for profs that want to view the annotations shared by students with them
  • Deal with special cases of media inheritance, e.g. media imported into another lecture
    -> who can see annotations... make consistent with what can be seen in the feedback player
  • Fix coloring: shared annotations shouldn't have the color from the annotation itself, but the category color
  • Use same icon in Thyme player for annotations than in the navbar
  • annotation.medium.teachable.lecture.title might trigger an exception since teachable is marked optional in Medium
  • Cypress tests

Won't fix

  • Default sorting -> I think the default sorting that Rails gives us is fine
  • Fix math-content not displayed properly if truncated before end of dollar sign -> do we really want to fix that? I don't think there's an easy solution to that... Use the following string in an annotation to see the problem:
and yet another one here with $m = a t + h$ content and a $\sum_{i=0}^n i = \pi^2$

$$a+b$$

$c+d+e+f+g$ whenever it gets very long
  • Notifications for new annotations? ->maybe something for the future

For reviewers

Important

Merge and review #669 beforehand

Don't be afraid by 700 lines of code changed, most of them are for Cypress tests.

  • Locally in dev, we only have one lecture with videos. Please add one manually to another one to test the functionality that we have multiple entries in the accordion. Note that in the Cypress tests, we have multiple lectures, so it's not a "problem" there.
  • Log in as student and as teacher to test different views (also done in Cypress tests)
  • Note that since the re-initialization of Cypress was performed very recently, I had to add/customize our current Cypress setup to accomodate for new needs that I didn't have in mind earlier on. I hope it's ok to have this in this PR as it would be a bit cumbersome to split it up into another PR first.

app/abilities/annotation_ability.rb Outdated Show resolved Hide resolved
app/helpers/media_helper.rb Show resolved Hide resolved
app/helpers/media_helper.rb Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
@Splines Splines merged commit 8e44e6f into dev Aug 13, 2024
5 checks passed
@Splines Splines deleted the feature/annotations-overview branch August 13, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants