Separate component for Recommendations #1133

vansika · 2020-10-10T10:29:52Z

Separate component to render recommendations.

vansika · 2020-10-10T10:31:46Z

@MonkeyDo @shivam-kapila I shared the screenshot on IRC, I don't understand why the recommendation card is not getting rendered, From the first look I understand that all the cards are overlapping each other, hence, we can see only one card, might be an issue of table reference, not sure.

listenbrainz/webserver/static/css/recommendation-page.less

shivam-kapila

This is a coarse review. I will review again once classes are over.

listenbrainz/webserver/static/css/recommendation-page.less

listenbrainz/webserver/static/js/src/recommendations/RecommendationCard.tsx

listenbrainz/webserver/static/js/src/recommendations/RecommendationControl.tsx

listenbrainz/webserver/static/js/src/recommendations/Recommendations.tsx

vansika · 2020-10-12T11:24:27Z

I have reused @shivam-kapila 's code in this PR at places, Thank you. I have tried to keep everything separate for recommendations (from less file to recommendation controls) so that it is easy to update things in future.

shivam-kapila

We need tests

listenbrainz/webserver/static/css/recommendation-page.less

listenbrainz/webserver/static/js/src/recommendations/RecommendationCard.tsx

listenbrainz/webserver/static/js/src/recommendations/Recommendations.tsx

shivam-kapila

LGTM overall. Tests needed.

listenbrainz/webserver/static/js/src/recommendations/RecommendationCard.tsx

MonkeyDo

Not sure if I was meant to review the styles of the recommendation card component, I had to modify similar_artist.html to point to recommendations.js instead of main.js in order to see them.

listenbrainz/webserver/static/css/recommendation-page.less

listenbrainz/webserver/static/js/src/recommendations/Recommendations.tsx

listenbrainz/webserver/static/js/src/recommendations/RecommendationCard.tsx

listenbrainz/webserver/static/css/recommendation-page.less

listenbrainz/webserver/static/js/src/recommendations/RecommendationCard.tsx

listenbrainz/webserver/static/js/src/types.d.ts

listenbrainz/webserver/static/css/recommendation-page.less

shivam-kapila · 2020-10-14T04:33:15Z

@vansika kindly run lint.sh to fix the build

vansika · 2020-10-14T17:05:02Z

@MonkeyDo I think I am happy with this PR now, I am not touching listened_at for Listens type (people freaking out), that means I will have to supply listened_at with recommendations which is okay for me. If you feel the changes are okay, I will want to write tests and merge it.

vansika · 2020-10-14T17:09:56Z

@MonkeyDo here is a screenshot for your reference :)

paramsingh

I think we should change BrainzPlayer to not require a listened_at prop instead of changing the recommendation type to keep a listened at there.

vansika · 2020-10-14T17:16:01Z

@paramsingh I will not be doing that in this PR. In another Pr, for sure

vansika · 2020-10-14T17:19:54Z

@paramsingh why not open a ticket :) will help me track

paramsingh · 2020-10-14T17:21:04Z

OK, sounds like a plan.

MonkeyDo

We'll need to go over the CSS, but it's not blocking for merging this PR.

MonkeyDo · 2020-10-15T09:09:02Z

listenbrainz/webserver/static/js/src/types.d.ts

+
+declare type Listen = BaseListenFormat & {
+  listened_at_iso?: string | null;
+  playing_now?: boolean | null;
  score?: number;


I wonder if this is relevant anywhere.
Doesn't need to be addressed in this PR, but maybe have a closer look at it when we tackle listened_at/BP changes.

MonkeyDo · 2020-10-15T09:09:36Z

listenbrainz/webserver/static/js/src/types.d.ts

  score?: number;
 };

+declare type Recommendation = BaseListenFormat;


That'll work nicely, and means you can easily extend the type with more fields as you need in the future, without having to change anything else( like we're gonna have to do for BP).
Yay future-proofing !

MonkeyDo · 2020-10-15T09:17:09Z

listenbrainz/webserver/static/css/recommendation-page.less

@@ -0,0 +1,61 @@
+#recommendations {
+  &:extend(#listens);
+  padding: 10px;


I think this less file will need a good refactor soon (and the listens-page file along with it), as there is still some duplicated css rules and a few things I'd like to change.
However, it ends up as just a few lines of extra unused CSS, which is not a blocker for this PR.
Considering how the rec styles are in flux, let's keep it for another PR.

MonkeyDo · 2020-10-15T09:25:14Z

listenbrainz/webserver/static/css/recommendation-page.less

+    > .col-xs-9, .col-xs-11 {
+
+      > .col-xs-9, .col-xs-11 {


I think you based this styling on what was there before, but a worthy comment:

Among the things that need changing that I mentioned above (for another PR), this use of bootstrap classes that are meant for layout to be used for styling should be replaced.
The reason is that for example if we decide to change the size of the columns but not the styling, we'll have to modify the less rules even though we haven't changed any of the styling.
Instead, we'll want them to be decoupled by using separate classes for styling and layout (like you do below with .smile, .laugh, .angry, .disappoint, .meh instead of > span).

MonkeyDo · 2020-10-15T09:38:22Z

listenbrainz/webserver/static/css/recommendation-page.less

+    padding: 15px;
+    margin-bottom: 7px;
+    max-height: 72px;


We'll have to review this before deploying. Spacing looks off to me:

don't modify bootstrap classes in recommendation-page.less

* recommendation componeent and recommendation card * CSS for recommendation component * shift recommendationPaginationControl to main method don't modify bootstrap classes in recommendation-page.less * fix utils.tsx error * html emoticons for rec feddback * update func name * revamp listens and recommendations type * tests for recommendation component * tests for rec component

* schema for storing recommendation feedback (#1126) * schema for recommendation feedback * rename enum strings and enum name * Separate component for Recommendations (#1133) * recommendation componeent and recommendation card * CSS for recommendation component * shift recommendationPaginationControl to main method don't modify bootstrap classes in recommendation-page.less * fix utils.tsx error * html emoticons for rec feddback * update func name * revamp listens and recommendations type * tests for recommendation component * tests for rec component * Refactor RecentListens - Remove recommendations code (#1146) * refactor recentListens * fix indentation * remove unused const * Recommendation feedback [DB + API] (#1143) * recommendation feedback model * use enum vals as feeeback to store in db * db and api script for recommendation feedback * PEP-8 fixes * tests feedback (db) * tests feedback (API) * tests update * don't create two objects * Sync recommendation feedback with emoticons (#1149) * tests feedback (db) * tests update * sync feedback * add missing props to test file * simplify get feedback for multiple recording query * keep recommendationFeedback type same as recommendationFeedback enum don't mutate the previous state * add event.stopPagination to stop closing of dropdown when submitting feedback * use Icon + text for feedback * use regular font awsome icons in place of stroke * if feedback given render corresponding feedback solid * remove redundant log statements * don't use prevState value in currRecPage * remove componentDidUpdate * adjust alignement of feedback for mobile view * capitalize feedback text * don't show feedbacks options for user != currentUser * define mediaquery after regular padding * tests for feedback * don't overwrite afterRecommendationDisplay in tests * add comment to instruct eslint to ignore import issue * tests for db * check for html elements before and after updating feedback * test interdependency of button and dropdown * format tests - nitpicks * remove bad_recommendation feddback type * adjust spacing between emoticons

vansika commented Oct 12, 2020

View reviewed changes

listenbrainz/webserver/static/css/recommendation-page.less Outdated Show resolved Hide resolved

vansika requested a review from shivam-kapila October 12, 2020 07:52

vansika changed the base branch from master to rec-component-feedback October 12, 2020 08:03

shivam-kapila suggested changes Oct 12, 2020

View reviewed changes

vansika changed the title ~~Refactor RecentListens.tsx: Separate component for Recommendations~~ Separate component for Recommendations Oct 12, 2020

vansika requested review from shivam-kapila and MonkeyDo October 12, 2020 11:24

vansika marked this pull request as ready for review October 12, 2020 12:01

shivam-kapila suggested changes Oct 13, 2020

View reviewed changes

vansika requested a review from shivam-kapila October 13, 2020 13:49

shivam-kapila reviewed Oct 13, 2020

View reviewed changes

listenbrainz/webserver/static/js/src/recommendations/RecommendationCard.tsx Show resolved Hide resolved

MonkeyDo requested changes Oct 13, 2020

View reviewed changes

MonkeyDo reviewed Oct 13, 2020

View reviewed changes

listenbrainz/webserver/static/css/recommendation-page.less Outdated Show resolved Hide resolved

paramsingh requested changes Oct 14, 2020

View reviewed changes

vansika requested a review from MonkeyDo October 15, 2020 06:55

MonkeyDo approved these changes Oct 15, 2020

View reviewed changes

vansika added 6 commits October 15, 2020 18:01

recommendation componeent and recommendation card

0cc4619

CSS for recommendation component

0569fd9

shift recommendationPaginationControl to main method

695bc08

don't modify bootstrap classes in recommendation-page.less

fix utils.tsx error

f65a011

html emoticons for rec feddback

6663edd

update func name

46e56f4

vansika added 2 commits October 15, 2020 18:01

revamp listens and recommendations type

6325ec4

tests for recommendation component

01a0ecc

vansika force-pushed the rec-card branch from b98f8bb to 01a0ecc Compare October 15, 2020 12:34

tests for rec component

ba41e8f

vansika merged commit dd7c379 into metabrainz:rec-component-feedback Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate component for Recommendations #1133

Separate component for Recommendations #1133

vansika commented Oct 10, 2020 •

edited

vansika commented Oct 10, 2020

shivam-kapila left a comment

vansika commented Oct 12, 2020

shivam-kapila left a comment

shivam-kapila left a comment

MonkeyDo left a comment

shivam-kapila commented Oct 14, 2020

vansika commented Oct 14, 2020 •

edited

vansika commented Oct 14, 2020

paramsingh left a comment

vansika commented Oct 14, 2020 •

edited

vansika commented Oct 14, 2020

paramsingh commented Oct 14, 2020

MonkeyDo left a comment

MonkeyDo Oct 15, 2020

MonkeyDo Oct 15, 2020

MonkeyDo Oct 15, 2020

MonkeyDo Oct 15, 2020

MonkeyDo Oct 15, 2020

Separate component for Recommendations #1133

Separate component for Recommendations #1133

Conversation

vansika commented Oct 10, 2020 • edited

vansika commented Oct 10, 2020

shivam-kapila left a comment

Choose a reason for hiding this comment

vansika commented Oct 12, 2020

shivam-kapila left a comment

Choose a reason for hiding this comment

shivam-kapila left a comment

Choose a reason for hiding this comment

MonkeyDo left a comment

Choose a reason for hiding this comment

shivam-kapila commented Oct 14, 2020

vansika commented Oct 14, 2020 • edited

vansika commented Oct 14, 2020

paramsingh left a comment

Choose a reason for hiding this comment

vansika commented Oct 14, 2020 • edited

vansika commented Oct 14, 2020

paramsingh commented Oct 14, 2020

MonkeyDo left a comment

Choose a reason for hiding this comment

MonkeyDo Oct 15, 2020

Choose a reason for hiding this comment

MonkeyDo Oct 15, 2020

Choose a reason for hiding this comment

MonkeyDo Oct 15, 2020

Choose a reason for hiding this comment

MonkeyDo Oct 15, 2020

Choose a reason for hiding this comment

MonkeyDo Oct 15, 2020

Choose a reason for hiding this comment

vansika commented Oct 10, 2020 •

edited

vansika commented Oct 14, 2020 •

edited

vansika commented Oct 14, 2020 •

edited