- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1
 
Heedls 516 group courses #539
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.
        
          
                ...lLearningSolutions.Web/Views/TrackingSystem/Delegates/DelegateGroups/_GroupCourseCard.cshtml
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...ningSolutions.Web/ViewModels/TrackingSystem/Delegates/DelegateGroups/GroupCourseViewModel.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...ningSolutions.Web/ViewModels/TrackingSystem/Delegates/DelegateGroups/GroupCourseViewModel.cs
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/DelegateGroups/GroupCourses.cshtml
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/DelegateGroups/GroupCourses.cshtml
              
                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 to me
| </summary> | ||
| <div class="nhsuk-details__text"> | ||
| <div class="tags"> | ||
| <div class="card-filter-tag"> | 
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.
Do we ever need to filter these?
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.
There's styles associated with this class to make tags on cards display properly
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.
Should we in this case name it something different (which doesn't refer to filters and should be used more generally)?
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.
Are you happy with the refactor of the css here? It is still slightly tied to the search/sort/paginate etc. styles
        
          
                DigitalLearningSolutions.Web/Views/TrackingSystem/Delegates/DelegateGroups/GroupCourses.cshtml
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | } | ||
| @if (Model.TotalPages > 1) | ||
| { | ||
| <partial name="SearchablePage/_Pagination" model="Model" /> | 
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.
Given that this is not searchable page, should the pagination partial folder have a different name/should it live in a different folder (as it's more generic)?
        
          
                ...ons.Web/ViewModels/TrackingSystem/Delegates/DelegateGroups/GroupCourseExpandableViewModel.cs
          
            Show resolved
            Hide resolved
        
              
          
                ...ons.Web/ViewModels/TrackingSystem/Delegates/DelegateGroups/GroupCourseExpandableViewModel.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data/Models/DelegateGroups/GroupCourse.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/DelegateGroupsController.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/DataServices/GroupsDataServiceTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Web/Controllers/TrackingSystem/Delegates/DelegateGroupsController.cs
          
            Show resolved
            Hide resolved
        
      8d3c607    to
    4574a45      
    Compare
  
    | 
           I've refactored the pagination to pull out the logic for that into a separate BasePaginationViewModel that the old BaseSearchablePageViewModel now implements. I've not done anything about the SortOptions on this ticket, I think we should revisit that on a ticket that is actually using search/filter but not sort.  | 
    
        
          
                DigitalLearningSolutions.Data/DataServices/GroupsDataService.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...ingSolutions.Web.Tests/Controllers/TrackingSystem/Delegates/DelegateGroupsControllerTests.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                DigitalLearningSolutions.Data.Tests/DataServices/GroupsDataServiceTests.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! 👍
JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-516
Description
Adds a page to display all the courses within the group as a paginated list of NHS expanders. (counterpart of the Delegates page from HEEDLS-513)
Screenshots
Developer checks
I have: