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

General: Add missing details to course detail view in course management #7397

Merged
merged 10 commits into from
Oct 23, 2023

Conversation

milljoniaer
Copy link
Contributor

@milljoniaer milljoniaer commented Oct 16, 2023

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I followed the coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

There have been some details missing in the course detail page. This PR adds this missing information.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 course
  1. Log in to Artemis
  2. Navigate to Course Detail View
  3. Verify that all necessary details of the course are displayed

Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Screenshots

image

@milljoniaer milljoniaer requested a review from a team as a code owner October 16, 2023 16:20
@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Oct 16, 2023
@milljoniaer
Copy link
Contributor Author

Thank you for your feedback, I implemented it, please have another look at the changes.

@milljoniaer milljoniaer temporarily deployed to artemis-test4.artemis.cit.tum.de October 19, 2023 10:54 — with GitHub Actions Inactive
julian-christl
julian-christl previously approved these changes Oct 19, 2023
Copy link
Member

@julian-christl julian-christl left a comment

Choose a reason for hiding this comment

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

Consider only showing the organisation section if there are any assigned to the course
Tested on TS 4, CR approved

max-bergmann
max-bergmann previously approved these changes Oct 19, 2023
Copy link
Contributor

@max-bergmann max-bergmann left a comment

Choose a reason for hiding this comment

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

Tested in testing session - changes mainly looks good to me

@julian-christl
Copy link
Member

Your build fails due to a client build error; the server tests also failed, I just reran them to be sure.

Copy link
Contributor

@dearjasmina dearjasmina left a comment

Choose a reason for hiding this comment

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

Tested on ts1, changes look good
The server tests fail, as soon as that's resolved you can contact me for approval (I've seen that the server tests fail in multiple PRs so maybe its another problem?)

@milljoniaer
Copy link
Contributor Author

I fixed the client build and since this PR does not include any server changes, I guess the falling tests are not connected to this PR.

Please have another look at the PR.

Copy link
Contributor

@b-fein b-fein left a comment

Choose a reason for hiding this comment

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

Thanks, code looks good now.

Copy link

@JanaNF JanaNF left a comment

Choose a reason for hiding this comment

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

Manual tested on legacy ts2. Worked as expected

Copy link

@Gusti2010 Gusti2010 left a comment

Choose a reason for hiding this comment

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

Manual tested on Ts3. Everything works as expected!

@milljoniaer milljoniaer temporarily deployed to artemis-test4.artemis.cit.tum.de October 23, 2023 09:21 — with GitHub Actions Inactive
Copy link
Member

@julian-christl julian-christl left a comment

Choose a reason for hiding this comment

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

Reapprove

Copy link
Contributor

@Strohgelaender Strohgelaender left a comment

Choose a reason for hiding this comment

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

code lgtm, just one optional (non-blocking) comment:

@krusche krusche changed the title General: Add missing details to course detail view General: Add missing details to course detail view in course management Oct 23, 2023
@krusche krusche added this to the 6.6.3 milestone Oct 23, 2023
@krusche krusche merged commit 6567a6b into develop Oct 23, 2023
58 of 62 checks passed
@krusche krusche deleted the chore/course-add-missing-details branch October 23, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants