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

[#9510] Instructor: Course Page/Student List Component: Implement sorting for table headers #9624

Conversation

Projects
None yet
5 participants
@pbrahmbhatt3
Copy link

commented Mar 30, 2019

Fixes #9510

Added sort functions in instructor-courses-page.components.ts to sort activeCourses, archivedCourses and softDeletedCourses.
Added buttons on in instructor-courses-page.component.html to sort the course list.

I would like to know more about how would you like to sort the studentList component as it is already sorted by team name.

@teammates-bot

This comment has been minimized.

Copy link

commented Mar 30, 2019

Hi @pbrahmbhatt3, these parts of your pull request do not appear to follow our contributing guidelines:

  1. PR Title
    • Issue Reference (#<issue-number>) missing.

@pbrahmbhatt3 pbrahmbhatt3 changed the title [Issue9510]Instructor: Course Page/Student List Component: Implement sorting for table headers <9510> Instructor: Course Page/Student List Component: Implement sorting for table headers Mar 30, 2019

@teammates-bot

This comment has been minimized.

Copy link

commented Mar 30, 2019

Hi @pbrahmbhatt3, these parts of your pull request do not appear to follow our contributing guidelines:

  1. PR Title
    • Issue Reference (#<issue-number>) missing.
@pbrahmbhatt3

This comment has been minimized.

Copy link
Author

commented Mar 30, 2019

Hi @pbrahmbhatt3, these parts of your pull request do not appear to follow our contributing guidelines:

1. PR Title
   
   * Issue Reference (`#<issue-number>`) missing.

I have updated PR's title to include issue number

@pbrahmbhatt3

This comment has been minimized.

Copy link
Author

commented Mar 30, 2019

@tanhengyeow I have updated the files correctly to fix the issue. However, I am not sure why the test is failing. Could please advice me in what could I correct to solve it! Your help will be much appreciated

@pbrahmbhatt3

This comment has been minimized.

Copy link
Author

commented Mar 30, 2019

I found out that there is an error in feedback-questions.service.ts.
image

It has nothing to do with the changes I have made. I am not sure how to fix it

@RonakLakhotia
Copy link
Contributor

left a comment

@pbrahmbhatt3 while the senior devs will review your feature , I can point out a few things.

  • Run the command gradlew generateTypes. Refer to the development workflow to be clear on the entire process.

  • You have lint erros in your code. Here is the build. Try and see the changes that are necessary in your code

@@ -48,9 +48,9 @@
},
"devDependencies": {
"@angular-builders/jest": "~7.1.2",
"@angular-devkit/build-angular": "~0.11.4",
"@angular-devkit/build-angular": "^0.13.7",

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Mar 30, 2019

Contributor

Revert this unrelated change

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Apr 13, 2019

Contributor

Also, don't forget to revert these changes

"@angular/cli": "~7.1.4",
"@angular/compiler-cli": "~7.1.4",
"@angular/compiler-cli": "^7.2.11",

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Mar 30, 2019

Contributor

Revert this too

<div class="float-right">
<h5 class="d-inline"><strong> Sort By: </strong></h5>
<div class="btn-group" data-toggle="buttons">
<button class="btn btn-light" (click)="sortById('archieve')">Course ID</button>

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Mar 30, 2019

Contributor

Not sure what you mean by archieve here, do you mean archive?

private statusMessageService: StatusMessageService,
private courseService: CourseService,
private modalService: NgbModal) { }
private httpRequestService: HttpRequestService,

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Mar 30, 2019

Contributor

Why this change in indentation?

@@ -208,7 +208,7 @@ export class InstructorCoursesPageComponent implements OnInit {
}, (resp: ErrorMessageOutput) => {
this.statusMessageService.showErrorMessage(resp.error.message);
});
}, () => {});
}, () => { });

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Mar 30, 2019

Contributor

Unrelated change

@@ -270,7 +305,7 @@ export class InstructorCoursesPageComponent implements OnInit {
this.statusMessageService.showErrorMessage(resp.error.message);
});

}, () => {});
}, () => { });

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Mar 30, 2019

Contributor

Unrelated change

sortPanelsBy(by: string):
((a: ActiveCourse, b: ActiveCourse) => number) {
return ((a: ActiveCourse, b: ActiveCourse): number => {
let strA: string;

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Apr 13, 2019

Contributor

These variables name don't seem to be intuitive. Perhaps use a more relatable name?

@@ -48,9 +48,9 @@
},
"devDependencies": {
"@angular-builders/jest": "~7.1.2",
"@angular-devkit/build-angular": "~0.11.4",
"@angular-devkit/build-angular": "^0.13.7",

This comment has been minimized.

Copy link
@RonakLakhotia

RonakLakhotia Apr 13, 2019

Contributor

Also, don't forget to revert these changes

@tanhengyeow
Copy link
Member

left a comment

Hi @pbrahmbhatt3

Thanks for working on this! The direction of this PR isn't as intended but good effort nonetheless. To clarify, here are two things we need to do:

  1. In the Instructor Course Page, we are not sorting the course tabs but rather sorting the course rows, so the behavior should not be replicated from the Instructor Home Page. In our previous version of the app, we are sorting just the first 3 columns and this would be good start:
    image

  2. The Instructor Course Details Page is using the student-list-component, and we should allow users to sort all the columns that have the sorting indicator:
    image

Once you have successfully implemented point 1, point 2 would be easy to implement as it is an extension. Let us know if you have any questions or unclear about anything 😄

@tanhengyeow tanhengyeow changed the title <9510> Instructor: Course Page/Student List Component: Implement sorting for table headers [#9510] Instructor: Course Page/Student List Component: Implement sorting for table headers Apr 18, 2019

@xpdavid

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Close due to inactivity.

@xpdavid xpdavid closed this Jun 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.