-
Notifications
You must be signed in to change notification settings - Fork 1
Heedls 468 basic centre ranking page #418
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor comments, but I am only halfway through a review before needing to leave for a long weekend. Submitting this review for now in case you want to tackle anything, but anyone else can review it instead if we want to get this ticket out of the way while I'm gone. Otherwise I'll pick it back up first thing on Tuesday.
| </thead> | ||
| <tbody class="nhsuk-table__body"> | ||
| @foreach (var centre in Model.TopTenCentres) { | ||
| <tr role="row" class="nhsuk-table__row @(centre.Rank == Model.CurrentCentre?.Rank ? "current-centre" : string.Empty)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I've encountered elements that sometimes need classes and sometimes don't, I've defined a variable at the top of the html in its own code block, rather than in the element attribute itself.
It is by no means a big deal, but do we have a preference one way or the other?
DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Centre/Ranking/CentreRankingViewModel.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest of the review completed. Few more comments.
...lLearningSolutions.Web.Tests/ViewModels/TrackingSystem/Centre/CentreRankingViewModelTests.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CentresDataService.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CentresDataService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
DigitalLearningSolutions.Web/Views/TrackingSystem/Shared/_NavMenuItems.cshtml
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Ranking/_RankingSuperAdminTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Ranking/_RankingSuperAdminTable.cshtml
Outdated
Show resolved
Hide resolved
...alLearningSolutions.Web/Views/TrackingSystem/Centre/Ranking/_RankingStandardUserTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/Centre/Ranking/_RankingSuperAdminTable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CentresDataService.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CentresDataService.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CentresDataService.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CentresDataService.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/ViewModels/TrackingSystem/Centre/Ranking/CentreRankViewModel.cs
Outdated
Show resolved
Hide resolved
...Solutions.Web.Tests/ViewModels/TrackingSystem/Centre/DashboardCentreDetailsViewModelTests.cs
Show resolved
Hide resolved
...lLearningSolutions.Web.Tests/ViewModels/TrackingSystem/Centre/CentreRankingViewModelTests.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data.Tests/Services/CentresServiceTests.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data.Tests/Services/CentresServiceTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍
Added the new Centre ranking page in the tracking system and the centre rank to the dashboard.
Current centre highlight is the colour chosen by Kevin, but can easily be updated if we need to change it in the future.
Normal admin page:

Super admin version showing the no data for your centre entry - this will always be true for test centre ID 101:

Mobile 3 col layout:

Centre dashboard with no activity. This displays the rank number if there has been activity at the centre in the last 14 days:
