-
Couldn't load subscription status.
- Fork 237
Implement recommendations display #4803
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
Implement recommendations display #4803
Conversation
…endations-display
…endations-display
…endations-display
Thanks @akolson! I will prioritize reviewing parts related to cards in this PR and follow-up on your Slack feedback on that opportunity. |
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.
Added a few questions/comments! I still want to do some manual testing but overall looking very good so far @akolson :)
| <ImportFromChannelsModal> | ||
| <template #default="{ preview }"> | ||
| <VSheet> | ||
| <div style="width: 1400px;"> |
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 know we are not super concerned with mobile responsiveness/optimization on studio, but I'm wondering if this needs to be somewhat more dynamically sized?
| :headingLevel="2" | ||
| :node="recommendation" | ||
| :selected.sync="selected" | ||
| @change_selected="handleChangeSelected" |
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 am not super familiar with using .sync so this may just be my own misunderstanding but I'm wondering why we have both a change selection event and a selection prop binding? are they doing different things? I'm also not clear on where handleChangeSelected is coming from
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.
The RecommendedResourceCard reuses existing functionality (the selected vuex state and handleChangeSelected handler) used by the main panel to allow for resource imports, as they do pretty much the same thing.
- The
selectedstate keeps track of what has been selected and deselected. - The
handleChangeSelectedhandler updates theselectedstate. - The
handleChangeSelectedreceives anisSelectedcallback from the clicked checkbox in the recommended resource card and passes back with the associated node selected or deselected - The
selectedprop is only passed for use in theisSelectedcallback whichI think we can directly declare in the recommended resources card to remove the ambiguity.
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...tion/contentcuration/frontend/RecommendedResourceCard/components/RecommendedResourceCard.vue
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.
Noting that this is only for illustration purposes on the frontend and will be removed once the integration with the recommendations API is in place
Hi @marcellamaki! Thanks for the review. I was able to work my way around the comments and have mades some tweaks. Happy to hear anymore thoughts, comments or suggestions. Thanks |
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'm confused where the backend API went and why it isn't connected here? The backend is what provides the recommendations, and so even without the API connected to the backend, I feel the backend should still be connected in this PR, unless we have another issue? I recall reviewing a PR for it, but I can't find it
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...tion/contentcuration/frontend/RecommendedResourceCard/components/RecommendedResourceCard.vue
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
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.
Leaving some thoughts
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Show resolved
Hide resolved
| on_delete=models.CASCADE, | ||
| ) | ||
| channel = models.ForeignKey( | ||
| Channel, |
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.
Since contentnode is associated with kolibri_public.models.ContentNode, I think it would make sense if this was also in alignment with kolibri_public. Although, I think as its written this seems best suited to be able to return the main_tree_id. Just more of a reflection and not a requested change.
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
| id: recommendation.id, | ||
| nodeId: recommendation.node_id, | ||
| rootId: recommendation.main_tree_id, | ||
| parent: recommendation.parent_id, |
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.
Where are we providing the these fields?
| rootId: recommendation.main_tree_id, | ||
| parent: recommendation.parent_id, | ||
| }); | ||
| const channel = await this.loadChannel(node.channel_id); |
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.
We might want to dedupe this.
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.
Tested and approved!
a5e978a
into
learningequality:search-recommendations
Summary
Description of the change(s) you made
This pr Implements the following
" page. See reference for details.
Manual verification steps performed
Screenshots (if applicable)
Good recommendations







Pagination - View more good recommendation
Pagination - Other (not so good) recommendations
Pagination - No good recommendations
Loading state
About recommendations
AI feature flag off
Are there any risky areas that deserve extra testing?
References
Closes #4565
Closes #4566
Closes #4683
Closes #4775
Comments
The
KCardcomponent is generally in a good place, however there could be few issues that could pop up here and there. For the latest changes you canyarn linkthe KDSdevelopbranch or use the latest release on npm, if up-to-date withdevelopContributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)