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

Update annotation form #4701

Merged
merged 24 commits into from
Aug 16, 2023
Merged

Update annotation form #4701

merged 24 commits into from
Aug 16, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Jun 1, 2023

This pull request restructures the annotation form in such a way that the saved annotations options don't take up to much extra space.

New:
image
Old:
image

New (save annotation):
image
Old (save annotation):
image

New (mobile):
image
Old (mobile):
image

Part of #4564

@jorg-vr jorg-vr added the enhancement A change that isn't substantial enough to be called a feature label Jun 1, 2023
@jorg-vr jorg-vr self-assigned this Jun 1, 2023
@jorg-vr jorg-vr marked this pull request as ready for review August 3, 2023 14:02
@jorg-vr jorg-vr requested a review from a team as a code owner August 3, 2023 14:02
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team August 3, 2023 14:02
@bmesuere bmesuere added the deploy mestra Request a deployment on mestra label Aug 4, 2023
@bmesuere bmesuere temporarily deployed to mestra August 4, 2023 06:30 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy mestra Request a deployment on mestra label Aug 4, 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.

Feedback from running this on metra:

  • the hover button text shows ask question instead of add comment (the global button does show add comment)
  • if there are no saved annotations, the area to the right of the textarea is left blank. I would add an empty state there or show the textarea full width.
  • if you check "save annotation", the title form is shown to the right of the text area. This somewhat breaks the flow. Maybe placing it inline after the checkbox + save annotation text would make more sense?
  • when save annotation is checked, it would make sense to change the text on the submit button to also say something that hints that the annotation will be saved.
  • the help text under the "search for annotations" combobox is too long (at least in Dutch)
  • in the search annotations input, the list of suggestions is not the same width as the text input
  • in the search annotations input, the list of suggestions "flickers" when searching
  • when picking a suggestion, the "save annotation" checkbox is disabled, but it is also shown as checked which might be confusion. Maybe replace it with a link to the page where you can edit that annotation?
  • when loading a saved annotation and then changing the text, a "not equals" sign is shown in the search annotation input. The icon is not aligned well (shown somewhere in the middle of the input field)

@jorg-vr jorg-vr marked this pull request as draft August 4, 2023 12:08
@jorg-vr jorg-vr mentioned this pull request Aug 4, 2023
17 tasks
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Aug 4, 2023

when picking a suggestion, the "save annotation" checkbox is disabled, but it is also shown as checked which might be confusion. Maybe replace it with a link to the page where you can edit that annotation?

This page does not exist yet (It is a modal currently)
I will just hide the checkbox for now and reintroduce that link when that page is created.
Added this note in #4564

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Aug 4, 2023

when save annotation is checked, it would make sense to change the text on the submit button to also say something that hints that the annotation will be saved.

I am not the biggest fan of this. I find it hard to come up with better wording for the button. A longer text is also makes it harder to properly layout the buttons (as they are on the same line as the checkbox currently)

The checkbox is also right next to the button, so the information is already provided on almost the same place

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Aug 4, 2023

the help text under the "search for annotations" combobox is too long (at least in Dutch)

I removed Saved annotations are automatically filtered by course and exercise.
This will change in version 2 anyway

@jorg-vr jorg-vr marked this pull request as ready for review August 7, 2023 12:51
@bmesuere bmesuere added deploy mestra Request a deployment on mestra and removed deploy mestra Request a deployment on mestra labels Aug 14, 2023
@bmesuere bmesuere temporarily deployed to mestra August 14, 2023 11:45 — with GitHub Actions Inactive
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 wording of "sla opmerking op om te hergebruiken" is quite verbose. Maybe make it "Opmerking opslaan" (maybe with an info icon with tooltip?). This would also help with my previous comment that it wasn't really clear that the comment would also be save when clicking the submit button
  • The "Titel:" label is a bit strange. Maybe floating labels are an option here?
  • Upon clicking "sla opmerking op", the titel input should get the focus
  • On smaller screens, the form layout isn't optimal:
    image

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Aug 16, 2023

The "Titel:" label is a bit strange. Maybe floating labels are an option here?

The floating labels don't work for our inputs, as we have a lot less vertical padding compared to the bootstrap default. Thus their is no vertical space for a label inside the input, without making the input look comically large compared to others.

I did make some custom styling based on the bootstrap css to put the label inside the input before the typing area
image

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

There's a JS error on mestra when signed in as staff. Otherwise, looks good 👍

updated(changedProperties: Map<string, any>): void {
// Focus the newly shown title input if the user wants to save the annotation.
if (changedProperties.has("saveAnnotation") && this.saveAnnotation) {
this.titleRef.value.focus();
Copy link
Member

Choose a reason for hiding this comment

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

@jorg-vr jorg-vr merged commit 8f1cbc0 into main Aug 16, 2023
13 checks passed
@jorg-vr jorg-vr deleted the enhance/reorganize-saved-annotation-form branch August 16, 2023 13:08
@jorg-vr jorg-vr temporarily deployed to naos August 16, 2023 13:08 — with GitHub Actions Inactive
@jorg-vr jorg-vr temporarily deployed to production August 16, 2023 13:12 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants