HEEDLS-435 - implemented Manage Course Page #506
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
AlexJacksonDS
left a comment
AlexJacksonDS
left a comment
There was a problem hiding this comment.
Just a few things about formatting/minor refactoring
| const int centreId = 101; | ||
| const int categoryId = 0; | ||
| var fixedCreationDateTime = DateTime.UtcNow; | ||
| var expectedCourseDetails = new CourseDetails |
Contributor
There was a problem hiding this comment.
Is it worth pulling this out into a helper?
| <dt class="nhsuk-summary-list__key"> | ||
| Password | ||
| </dt> | ||
| <partial name="_SummaryFieldValue" model="@Model.Password" /> |
Contributor
There was a problem hiding this comment.
I don't think we need to use the partial here since there is already a condition around the Password being null or white space.
| <dt class="nhsuk-summary-list__key"> | ||
| Course completion notification email | ||
| </dt> | ||
| <partial name="_SummaryFieldValue" model="@Model.NotificationEmails" /> |
Comment on lines
17
to
19
| <dd class="nhsuk-summary-list__value"> | ||
| @(Model.Active ? "Yes" : "No") | ||
| </dd> |
Contributor
There was a problem hiding this comment.
Might be worth turning these bool to Yes/No into a partial similar to the value with a dash partial
| <div class="nhsuk-card__content basic-card-content"> | ||
| <dl class="nhsuk-summary-list"> | ||
| <div class="nhsuk-summary-list__row basic-summary-list__row"> | ||
| <dt class="nhsuk-summary-list__key">Current version</dt> |
Contributor
There was a problem hiding this comment.
I'm sure you've run the formatter here, but is there any other reason for these to all be single line rather than the spaced version we're using elsewhere.
| <dl class="nhsuk-summary-list"> | ||
| <div class="nhsuk-summary-list__row basic-summary-list__row"> | ||
| <dt class="nhsuk-summary-list__key">Current version</dt> | ||
| <dd class="nhsuk-summary-list__value ">@Model.CurrentVersion</dd> |
Contributor
There was a problem hiding this comment.
There are also a number of classes with a space at the end in this file
Contributor
AlexJacksonDS
left a comment
AlexJacksonDS
left a comment
There was a problem hiding this comment.
Just one thing left to change
Comment on lines
73
to
75
| <dd class="nhsuk-summary-list__value"> | ||
| @(Model.ApplyLpDefaultsToSelfEnrol ? "Yes" : "No") | ||
| <partial name="_SummaryBooleanField" model="Model.ApplyLpDefaultsToSelfEnrol"/> | ||
| </dd> |
Contributor
There was a problem hiding this comment.
Shouldn't this whole element have been replaced?
AlexJacksonDS
approved these changes
Jul 26, 2021
stellake
reviewed
Jul 26, 2021
....Web/Views/TrackingSystem/CourseSetup/ManageCourse/_LearningPathwayDefaultsExpandable.cshtml
Outdated
Show resolved
Hide resolved
....Web/Views/TrackingSystem/CourseSetup/ManageCourse/_LearningPathwayDefaultsExpandable.cshtml
Outdated
Show resolved
Hide resolved
....Web/Views/TrackingSystem/CourseSetup/ManageCourse/_LearningPathwayDefaultsExpandable.cshtml
Outdated
Show resolved
Hide resolved
...earningSolutions.Web/Views/TrackingSystem/CourseSetup/ManageCourse/_CourseSummaryCard.cshtml
Outdated
Show resolved
Hide resolved
...gSolutions.Web/Views/TrackingSystem/CourseSetup/ManageCourse/_CourseDetailsExpandable.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/Views/TrackingSystem/CourseSetup/AdminFields/AdminFields.cshtml
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Web/ViewModels/TrackingSystem/CourseSetup/AdminFieldsViewModel.cs
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CourseDataService.cs
Outdated
Show resolved
Hide resolved
DigitalLearningSolutions.Data/DataServices/CourseDataService.cs
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-435
Description
Added new Manage Course page in the course setup. Manage Admin Fields page has had some breadcrumbs/back links changed to point to this new page. Note that the final two buttons are secondary buttons rather than primary buttons (used in the ticket wireframes) at Stella's suggestion to reduce the amount of green buttons on the page.
Screenshots
Desktop example 1:
Mobile version of example 1:
Desktop example 2:
Learning Pathway defaults with same course name:
Course Options with PLAssess = 0:
Developer checks
I have: