Skip to content

Conversation

@DanBloxham-sw
Copy link
Contributor

@DanBloxham-sw DanBloxham-sw commented Aug 18, 2021

...emented

JIRA link

This is split over three tickets:
Search & Paginate: https://softwiretech.atlassian.net/browse/HEEDLS-432
Sort: https://softwiretech.atlassian.net/browse/HEEDLS-433
Filter: https://softwiretech.atlassian.net/browse/HEEDLS-434

Description

Implemented both JS and non-JS versions of searching, pagination, sorting, and filtering to the Course Setup page.

Screenshots

JS UI with full page:
Desktop js search (2)

Non JS UI:
Desktop no js

Mobile:


Developer checks

I have:

  • Run the formatter and made sure there are no IDE errors.
  • Written tests for the changes (controller, data services, services, view models etc) and manually tested my work with and without JavaScript. Full manual testing guidelines can be found here: https://softwiretech.atlassian.net/wiki/spaces/HEE/pages/6703648740/Testing
  • Updated/added documentation in Swiki and/or Readme.
  • Updated my Jira ticket with information about other parts of the system that were touched as part of the MR and have to be sanity tested to ensure nothing’s broken.
  • Scanned over my own MR to ensure everything is as expected.

Copy link
Contributor

@AlexJacksonDS AlexJacksonDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor things, other than those it looks OK to me

Comment on lines 29 to 30
ApplicationName = "Course", CustomisationName = "Customisation", Active = true, CourseTopic = "Topic 1",
CategoryName = "Category 1", HideInLearnerPortal = true, DelegateCount = 1, CompletedCount = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This initialiser shouldn't be formatted like this

private readonly ICourseService courseService;

public CourseSetupController(ICourseService courseService)
public CourseSetupController(ICourseService courseService, ICourseCategoriesDataService courseCategoriesDataService, ICourseTopicsDataService courseTopicsDataService)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line, run formatter

return View(model);
}

private void UpdateFilterCookie(string? filterBy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a slightly different approach to this comment and only took out the contents of the true branch to a new method. We should be consistent, but I'm not sure which way.

If we take the approach here, the method needs renaming to something referencing the fact that it may also be deleted here.

Copy link
Contributor

@stellake stellake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Could you please double check before merge that the VC inconsistency is mentioned in JIRA? 🙏

@DanBloxham-sw DanBloxham-sw merged commit a393215 into master Aug 24, 2021
@DanBloxham-sw DanBloxham-sw deleted the HEEDLS-432-CentreCourseSetupSearchAndPagination branch August 24, 2021 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants