HEEDLS-470 - Implemented Top Courses page #463
Conversation
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Outdated
Show resolved
Hide resolved
...alLearningSolutions.Web/Controllers/TrackingSystem/Centre/TopCourses/TopCoursesController.cs
Outdated
Show resolved
Hide resolved
...alLearningSolutions.Web/Controllers/TrackingSystem/Centre/TopCourses/TopCoursesController.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Outdated
Show resolved
Hide resolved
| private readonly ICourseService CourseService; | ||
| private const int NumberOfTopCourses = 10; |
There was a problem hiding this comment.
Is this naming change intentional?
There was a problem hiding this comment.
Sorry, highlighted the wrong line. The CourseService
There was a problem hiding this comment.
With the configuration -> Configuration change on the view, I changed my resharper settings so that it would be preferred as uppercase rather than lowercase (Code editing -> C# -> Naming style -> Instance Fields (private) = UpperCamelCase rather than lowerCamelCase). This then suggested that courseService should be CourseService. Pretty sure that most other files use lowerCamelCase for these private fields, so I'm not sure why we are only seeing this issue now.
There was a problem hiding this comment.
Yeah the setting for at least us (and maybe everyone else except Stella?) says that it should be courseService & configuration.
Whereas Stella was getting Fields (not private) errors with lower case configuration
There was a problem hiding this comment.
Well I'll just stick with what we've got for other files then. Reverted CourseService back to courseService. Kept Configuration as Configuration.
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/TopCourses/Index.cshtml
Show resolved
Hide resolved
| <span class="nhsuk-table-responsive__heading">Delegates </span>@course.DelegateCount | ||
| </td> | ||
| <td role="cell" class="nhsuk-table__cell nhsuk-u-padding-left-2 cell-right-padding"> | ||
| <span class="nhsuk-table-responsive__heading">Pass rate </span>@course.PassRate % |
There was a problem hiding this comment.
This is super minor, but the old site does not have space before the "%" symbol
| this.courseDataService = courseDataService; | ||
| } | ||
|
|
||
| public IEnumerable<CourseStatistics> GetTopCourseStatisticsAtCentreForCategoryId(int centreId, int categoryId) |
There was a problem hiding this comment.
I think the name "GetTopCourseStatistics" would be sufficient as the centreId & cetegoryId are already present in params (just to make the name a bit easier to read).
DigitalLearningSolutions.Data/Models/Courses/CourseStatistics.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/Models/Courses/CourseStatistics.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data.Tests/DataServices/CourseDataServiceTests.cs
Show resolved
Hide resolved
| cu.CentreID, | ||
| cu.Active, | ||
| cu.AllCentres, | ||
| ap.ASPMenu, |
There was a problem hiding this comment.
Should we still pull this?
There was a problem hiding this comment.
Good spot, forgot to get rid of it here. Will do that now.
3 participants
Ran all unit tests.
Tested in Chrome, IE11, Firefox, Edge.
Tested with mobile view/zoom, screen reader, google lighthouse
I've left a few comments on relevant bits of code to highlight potential issues.
Screen shots:
Desktop:
Mobile:
