-
Notifications
You must be signed in to change notification settings - Fork 735
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
Recommended resources issue 1713 #1720
Recommended resources issue 1713 #1720
Conversation
@filter_tags = @matched_tags.map do |t| | ||
"filter[]=#{t}&" | ||
end | ||
@filter_tags = @filter_tags.join() |
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.
Is this line necessary? Nothing is being joined to @filter_tags.
tag.tr('_', '-') | ||
end | ||
end | ||
matched_tags = (@moment_keywords & resource_tags) |
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 be returning @moment_keywords & resource_tags
instead of setting it to the matched_tags
variable?
Hello @nshki, @julianguyen, and other reviewers. We have updated our code according to the initial code reviews, and all local and external tests currently pass. Please let us know if you have any questions or concerns. Kind regards, TeamBits :) |
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.
Solid work! Tests are great too!
Just a few more things and we should be ready to merge :)
app/views/moments/show.html.erb
Outdated
@@ -43,6 +43,18 @@ | |||
</div> | |||
<% end %> | |||
|
|||
<% if @resources.any? %> | |||
<div class="smallMarginTop"> | |||
<div class="label"><%= label_tag "What resources could help?" %> </div> |
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 unnecessary whitespace after the label_tag
we should remove so that it looks like <div class="label"><%= label_tag "What resources could help?" %></div>
.
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 also realized that this text should be in our translation files. Feel free to use Google Translate 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.
Hi Julia! Thank you for your comments. I do have 2 questions, please:
- With regards to the white space, I'm not sure which space you are referring to? To me the code and your example look exactly the same. Are you referring to the indent space?
app/views/moments/show.html.erb
Outdated
<% @resources.take(3).each do |item| %> | ||
<li><%= link_to item['name'], item['link'] %></li> | ||
<%end %> | ||
<li><%= link_to 'More...', "/resources?#{@resources_tags}" %></li> |
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.
"More..." should be in our translation files. Alternatively, you could use the existing t('load_more')
key. If you decide to translate a new key, feel free to use Google Translate for now.
…ecommended_resources_issue_1713 Rebase master with branch before push
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.
Excellent work! :D This came together well 🎉
Thanks again for tackling this!
Thank you @julianguyen, @nshki and @S-Warmenhoven ! |
Description
Based on what a user has inputted in their Moment post (including tagged Categories, Moods, and Strategies), keywords are extracted and matched with Resource tags to display up to 3 recommended resources on the Moment Show Page. Additionally, there is a "More..." link to redirect the user to the Resources Index Page.
We created two classes called MomentKeywords and ResourceRecommendation that are called in the moments_controller for each Moment.
More Details
This is Phase #1 of this feature. There are some other ways to make the recommendations more optimal if necessary for further phases, such as using Text processing tools.
Further suggestions for the Recommendation engine:
Also, with the "More..." link, certain tags could be pre-selected too when directed to the index page.
Corresponding Issue
Issue [#1713]
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!