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

[#9508] Improve UI for Instructor Course Details #9602

Merged
merged 21 commits into from Apr 8, 2019
Merged

[#9508] Improve UI for Instructor Course Details #9602

merged 21 commits into from Apr 8, 2019

Conversation

ritisha2000
Copy link
Contributor

@ritisha2000 ritisha2000 commented Mar 23, 2019 •
edited

Fixes #9508

I found the file instructor-course-details-page.component.html in pages-instructor\instructor-course-details-page\instructor-course-details-page.component.html and removed the student header. I also changed the card-body class p-3 to p-0 align it to the card body above.

tanhengyeow
tanhengyeow reviewed Mar 23, 2019
View reviewed changes
Copy link
Member

@tanhengyeow tanhengyeow left a comment •
edited

Choose a reason for hiding this comment

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

Hi @ritisha2000

Just some quick comments before your PR is ready for review.

  1. You shouldn't need to update src/test/resources/pages/* files. Those are legacy files that we are trying to deprecate.

  2. Please look at the Travis output to find out what is causing the build to fail.

  3. Please attach a screenshot of how the new UI looks like to facilitate reviews.

@tanhengyeow tanhengyeow self-assigned this Mar 23, 2019
@tanhengyeow tanhengyeow added the s.Ongoing The PR is being worked on by the author(s) label Mar 23, 2019
@ritisha2000
Copy link
Contributor Author

CoureDetails

I checked what the error was and it is failing a test because the jest snapshot is not updated and it is comparing it to the old snapshot which contains the student div. I am working on changing that but here's what the edits look like. Thank you for your feedback.

@amrut-prabhu amrut-prabhu mentioned this pull request Mar 24, 2019
Closed
tanhengyeow
tanhengyeow requested changes Mar 25, 2019
View reviewed changes
Copy link
Member

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

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

Good job in working on this! @ritisha2000

Refer to some of the comments made. As a general rule of thumb, we do not want to introduce unrelated changes with respect to the issue that the PR is trying to solve. There are some exceptions but in this case it is not :)

package.json Outdated Show resolved Hide resolved
.../app/pages-instructor/instructor-course-edit-page/instructor-course-edit-page.component.html Outdated Show resolved Hide resolved
...ages-instructor/instructor-course-details-page/instructor-course-details-page.component.html Outdated Show resolved Hide resolved
@tanhengyeow
Copy link
Member

Also, although not strictly required, it would be nice to tidy up your PR description.

Here are some examples below:

  1. We always put Fixes #.... at the top of the description.

  2. This chunk of info seems irrelevant:

…artinoamigo/teammates into 9508_Course_Details_Improve_UI

[#9508] Instructor: Course Details: Improve UI
  1. It's good to use backticks to differentiate file names and file paths e.g. instructor-course-details-page.component.html and pages-instructor\instructor-course-details-page\instructor-course-details-page.component.html (just the relative path would suffice)

@ritisha2000
Copy link
Contributor Author

There were two unnecessary changes so I removed them and changed the description. Thank you for your help.

tanhengyeow
tanhengyeow requested changes Apr 1, 2019
View reviewed changes
Copy link
Member

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

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

@ritisha2000 Close to merging :)

Please refer to the comment made.

src/test/resources/pages/instructorCourseDetailsWithSections.html Outdated Show resolved Hide resolved
tanhengyeow
tanhengyeow approved these changes Apr 4, 2019
View reviewed changes
Copy link
Member

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your first contribution 🎉 @ritisha2000

@tanhengyeow tanhengyeow added the s.FinalReview The PR is ready for final review label Apr 4, 2019
@ritisha2000
Copy link
Contributor Author

Thank you for all your help

@amrut-prabhu amrut-prabhu removed the s.Ongoing The PR is being worked on by the author(s) label Apr 5, 2019
wkurniawan07
wkurniawan07 approved these changes Apr 7, 2019
View reviewed changes
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@tanhengyeow tanhengyeow added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 8, 2019
@amrut-prabhu amrut-prabhu merged commit cd84929 into TEAMMATES:master Apr 8, 2019
@wkurniawan07 wkurniawan07 added this to the V7.0.0-beta.0 milestone Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wkurniawan07 wkurniawan07 wkurniawan07 approved these changes

@tanhengyeow tanhengyeow tanhengyeow approved these changes

Assignees

@tanhengyeow tanhengyeow

Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V7.0.0-beta.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ritisha2000 @tanhengyeow @wkurniawan07 @amrut-prabhu @okasha30495 @okasha95