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.

Sign up for GitHub

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

Jump to bottom

Development: Document exam exercise update notifications #7294

Merged
merged 13 commits into from
Oct 14, 2023
Merged

Development: Document exam exercise update notifications #7294

merged 13 commits into from
Oct 14, 2023

Conversation

tobias-lippert
Copy link
Contributor

@tobias-lippert tobias-lippert commented Sep 29, 2023 •
edited
Loading

Checklist

General

Motivation and Context

The exam exercise update notification feature lacks documentation

Description

Added documentation including screenshots.

Steps for Testing

Have a look at the docs
Code Review

Review Progress

Check the docs and the added information to the feature overview (shown in screencasts below)

Code Review

  • Code Review 1
  • Code Review 2

Screenshots

exam-exercise-update-highlighter-docs.mp4

@tobias-lippert tobias-lippert requested a review from a team as a code owner September 29, 2023 11:12
MaximilianAnzinger
MaximilianAnzinger previously approved these changes Sep 29, 2023
View reviewed changes
Copy link
Collaborator

@MaximilianAnzinger MaximilianAnzinger left a comment

Choose a reason for hiding this comment

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

LGTM one suggestion: Maybe it should be more obvious that manual merging is not possible in the online editor? Some highlighting since this might be a problem?

florian-glombik
florian-glombik requested changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@florian-glombik florian-glombik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some comments on the formulations, the error with the headline must be fixed to ensure that the documentation compiles

docs/user/exams/instructors_guide.rst Show resolved Hide resolved
docs/user/exams/instructors_guide.rst Outdated Show resolved Hide resolved
docs/user/exams/instructors_guide.rst Outdated Show resolved Hide resolved
docs/user/exams/students_guide.rst Outdated Show resolved Hide resolved
laurenzfb
laurenzfb previously approved these changes Sep 30, 2023
View reviewed changes
Copy link
Contributor

@laurenzfb laurenzfb left a comment

Choose a reason for hiding this comment

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

Overall LGTM but I do agree on the suggested changes by @florian-glombik

aplr
aplr previously approved these changes Sep 30, 2023
View reviewed changes
Copy link
Contributor

@aplr aplr left a comment

Choose a reason for hiding this comment

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

LGTM w/ change requests from @florian-glombik also. Good to read & understandable otherwise.

@krusche krusche changed the title General: Document Exam Exercise Update Notifications Development: Document exam exercise update notifications Oct 3, 2023
b-fein
b-fein previously approved these changes Oct 7, 2023
View reviewed changes
laurenzfb
laurenzfb previously approved these changes Oct 8, 2023
View reviewed changes
Copy link
Contributor

@laurenzfb laurenzfb left a comment

Choose a reason for hiding this comment

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

LGTM after changes!

b-fein
b-fein previously approved these changes Oct 8, 2023
View reviewed changes
aplr
aplr previously approved these changes Oct 9, 2023
View reviewed changes
@tobias-lippert tobias-lippert dismissed stale reviews from b-fein and laurenzfb via 420b9ce October 9, 2023 20:30
florian-glombik
florian-glombik previously approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@florian-glombik florian-glombik left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the feedback! Looks good to me now!

b-fein
b-fein previously approved these changes Oct 10, 2023
View reviewed changes
aplr
aplr previously approved these changes Oct 10, 2023
View reviewed changes
laurenzfb
laurenzfb previously approved these changes Oct 10, 2023
View reviewed changes
krusche
krusche requested changes Oct 10, 2023
View reviewed changes
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

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

Could you also document this feature on https://artemis.in.tum.de/features/instructors and on https://artemis.in.tum.de/features/students ?

And does it make sense to put the exam announcement and time extension in this PR as well?

@tobias-lippert
Copy link
Contributor Author

tobias-lippert commented Oct 11, 2023 •
edited
Loading

@krusche Gonna add that. Regarding the second aspect, I do not know much about the exam announcement feature but as far as I know they work still a bit differently at the moment.
@aplr What do you think? Do you want to add your documenation to this PR?

@aplr
Copy link
Contributor

aplr commented Oct 12, 2023

@krusche @tobias-lippert If you don't have opposing views, I'd prefer to add time extensions & announcements in another PR.

@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Oct 12, 2023
@krusche
Copy link
Member

krusche commented Oct 12, 2023

@krusche @tobias-lippert If you don't have opposing views, I'd prefer to add time extensions & announcements in another PR.

As long as we document this asap, I'm fine

@tobias-lippert tobias-lippert added this to the 6.6.0 milestone Oct 13, 2023
@krusche krusche merged commit 3900fea into develop Oct 14, 2023
35 of 39 checks passed
@krusche krusche deleted the documenation/exam-exercise-update-notification branch October 14, 2023 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krusche krusche krusche requested changes

@b-fein b-fein b-fein approved these changes

@MaximilianAnzinger MaximilianAnzinger MaximilianAnzinger approved these changes

@laurenzfb laurenzfb laurenzfb approved these changes

@florian-glombik florian-glombik florian-glombik left review comments

@aplr aplr aplr left review comments

Assignees

@tobias-lippert tobias-lippert

Labels
client Pull requests that update TypeScript code. (Added Automatically!) documentation ready to merge
Projects
None yet
Milestone
6.6.0
Development

Successfully merging this pull request may close these issues.
7 participants
@tobias-lippert @aplr @krusche @laurenzfb @MaximilianAnzinger @florian-glombik @b-fein