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 creation of user annotations based on a selection #4596

Merged
merged 61 commits into from
May 17, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Apr 25, 2023

This pull request marks user annotations inside the code. It uses a background instead of a text decoration. This way one machine annotation and one user annotation can be visualized simultaneously. The most important type of each will get priority.

It is possible to mark part of a line, or multiple whole lines. If a line is only partially selected in a multi line selection, the selection will be adjusted to select the whole line.

Examples:
image

While selecting code, selecting other text or annotations is disabled. This makes it easier to select code across annotations. This also fixes #3976 . It is still possible to select text within an annotation if your selection starts there. Code selections and other selections are differentiated by the selection color and the side button becoming larger.

Peek 2023-05-15 14-51

Using the old create annotation button marks the whole line:
Peek 2023-05-15 14-48

When all annotations are hidden, the marked annotations have tooltips:
Peek 2023-05-15 14-49

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

Note: Testing was hard as firefox implements selection differently than chrome. The testing framework works similar to chrome, so the firefox code could not be tested.

Closes #3976 and #2826

@jorg-vr jorg-vr added the feature New feature or request label Apr 25, 2023
@jorg-vr jorg-vr self-assigned this Apr 25, 2023
@jorg-vr jorg-vr added the deploy mestra Request a deployment on mestra label Apr 27, 2023
@jorg-vr jorg-vr temporarily deployed to mestra April 27, 2023 14:06 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Apr 27, 2023
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label May 9, 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.

  • Regarding the button: why not make this a proper FAB? The collapsed version could then expand on mouseover instead of the tooltip now.

  • Would it be possible to also drag the button to select lines? Drag release would then automatically trigger the button click which would be the fasted way to add a multi line comment: big click target to start drag, drag a few lines, release and directly start typing.

  • The background color while selecting is ok, but seems a bit strange after adding the comment. For comments of a substring, I would expect it to mimic the curly underline of the linting comments. For single or multi-line comments it seems very prominent. Alternatives: show something in the margin or show no formatting in the code but add a "comment on line x" "comment on line x to y" to the top of the comment (which is what gitlab does). Mouseover on the line(s) or comment could then show the range using a bg-color.

@jorg-vr
Copy link
Contributor Author

jorg-vr commented May 10, 2023

@bmesuere does the following match your expectations for the FAB buttons:
image
image
image
image

I used the fab-small style from the spec and created a corresponding fb-small-extended which does not follow spec. But fab and fab-extended spec was really large in this context

Sidenote: in this pr #4616 I was experimenting with a design that more closely followed the google docs design to work with multiple buttons. Which looked like this:
image
image
(Always centered on the table border)
I did not yet solve how to indicate the functional difference with a selection in this case.

@jorg-vr jorg-vr requested a review from bmesuere May 15, 2023 12:56
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label May 16, 2023
@bmesuere bmesuere temporarily deployed to mestra May 16, 2023 18:12 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label May 16, 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.

looks good, some nitpicks:

  • I think it will look better if you keep the icon left and put the text on the left. Maybe a little css transition to expand the button will also help with the smoothness
  • on mouse over, the button in slightly darker (in light mode), however, the little arrow stays in the base color
  • dragging the FAB doesn't work for me (chrome). I can drag the button (both in x- and y-direction, where I would expect the x-position to be locked), but when I release it, the button is gone and no form appears. In addition, the button also doesn't show up when I hover over the code. There is no error in the console.
  • the default line highlights look more subtle now, well done!
  • the darker highlight on mouse over is also very nice!

@bmesuere
Copy link
Member

bmesuere commented May 16, 2023

  • Maybe we can move the button drag behaviour to a later PR to be able to make a release tomorrow.

@bmesuere bmesuere requested a review from niknetniko May 16, 2023 18:43
@jorg-vr
Copy link
Contributor Author

jorg-vr commented May 17, 2023

@bmesuere I did not add the transition because I use the display properties right now got the button change, which don't work with css transitions.
For transitions I would need to use something like visibility, but this would also require a rework of the html structuring and some nit picky positioning.

I will work on this after the release if desired.

@jorg-vr jorg-vr requested a review from bmesuere May 17, 2023 08:01
@jorg-vr jorg-vr merged commit 643d393 into develop May 17, 2023
@jorg-vr jorg-vr deleted the feat/range-annotations branch May 17, 2023 11:21
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.

Selecting code also includes the annotations
5 participants