- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1
 
HEEDLS-650 - Adding searching and pagination to the Recommended Learn… #876
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
HEEDLS-650 - Adding searching and pagination to the Recommended Learn… #876
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.
A couple of questions, but looks mostly fine
        
          
                ...lLearningSolutions.Web/Controllers/LearningPortalController/RecommendedLearningController.cs
          
            Show resolved
            Hide resolved
        
      | public AllRecommendedLearningItemsViewModel(IEnumerable<RecommendedResource> resources, int selfAssessmentId) | ||
| { | ||
| RecommendedResources = | ||
| resources.OrderByDescending(r => r.RecommendationScore).Select(r => new SearchableRecommendedResourceViewModel(r, selfAssessmentId)); | 
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 need the order by here? Or is this to make the javascript ordering work as expected?
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.
Without it, the JS ordering doesn't work properly. But that is only because I have no properly set up the JS ordering as we would expect.
We need to order based on the Recommendation Score, but we don't currently pass that through to card (only the wording for the tag). I thought that, rather than adding some hidden values to each card and setting up the SortOptions, I'd just order them in this view model first. The JS manipulation then preserves the order as it is because it doesn't have anything to sort on.
It's only a small reduction in the code necessary, and not a big time sink to implement the usual way, so I'm happy to implement that if you think it'd be better than using this workaround.
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.
Fair enough, I don't really thing it matters either way, so lets just leave it for now
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.
All looks good to me, I'm just interested to know why we don't want caching on certain endpoints.
| return competencies; | ||
| } | ||
| 
               | 
          ||
| [NoCaching] | 
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.
Just for my benefit, why do we need NoCaching?
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.
I was going to link the thread but realised it was in the signposting channel you don't have access to.
This tackles the drive-through bug fix in the ticket description. In short, when going through the self assessment journey, clicking some links causes the current page content to be temporarily replaced with a loading-spinner while waiting for the response for the next page. If you reach the next page, and then hit the back button, the previous page is recovered from the browser cache to display, rather than making a call to the action. Because the cache has the latest version of the page, it has the loading spinner replace all the content on that page, making it useless until you refresh the page. NoCaching just cuts out the need to refresh, as it forces the browser to always make a call to the action.
…ing page
JIRA link
https://softwiretech.atlassian.net/browse/HEEDLS-650
Description
Added searching and pagination to the Recommended Learning page.
Screenshots
Desktop:
Desktop (No JS):
Tablet:
Mobile:
Developer checks
(Leave tasks unticked if they haven't been appropriate for your ticket.)
I have: