Personal Recommendation Frontend

[hrik2001]

Problem

The backend for personal recommendation has already been merged, but to recommend, we need a frontend, this PR (draft PR for now) attempts to solve that.

Solution

Creating a modal and choosing followers to whom you want to recommend. UI/UX of this PR has been followed according to the GSoC proposal for the same

[MonkeyDo]

@hrik2001 Hi! Is this ready for another review? If not, anything I can do to help?

[hrik2001]

Hi @MonkeyDo, the PR is ready for review, very very sorry for leaving you hanging, I was down with covid apparently, but I have recovered! I couldn't even attend the MB summit stream :/

The new commits do everything you asked for except of the shift to mbid and the isFunction function that you suggested. Opted it out for now because jest's mock function doesn't get qualified as a function.

[hrik2001]

Also @MonkeyDo, I do need your help in one thing. For some reason, after merging the master, I have errors in frontend tests (and those errors don't appear to be happening in the tests here in github). Due to this reason, I am not able to generate snapshots. (You can see the only tests that are failing are snapshot tests)

[MonkeyDo]

Hi @MonkeyDo, the PR is ready for review, very very sorry for leaving you hanging, I was down with covid apparently, but I have recovered! I couldn't even attend the MB summit stream :/

Oh damn, I hope it wasn't too hard on you! Glad to hear you're doing better though. Sorry if I sounded pushy, I just wanted to make sure the PR doesn't end in limbo as sometimes happens; I'm just so much looking forward to using the feature ! :)

I'll do another review pass and look at the tests. Without having looking at anything, I'd wager it's probably related to some recent package updates I've been working on, so might require rebuilding some docker images (./develop.sh build web static_builder).

[MonkeyDo]

The tests issue was probably what I mentioned above, related to recent changes to ESLint and Webpack. It all worked for me without the need for any change, but then again my local repo had all the new libraries already installed.

I took the liberty to push a couple of mini-changes while I was reviewing: a couple of things related to the same recent ESLint changes and a small phrasing change on an error message. I updated the snapshots (since I ran the suite to see if it would work for me :p )

[MonkeyDo]

Well, I was surprised and a bit confused by the comment about isFunction and the jest mocks, because I was expecting jest mock functions to pass as regular functions. So I tried it…and it works! Not sure what issue you hit when you tried it out, but I took the liberty of pushing those extra changes.

[hrik2001]

Hi @MonkeyDo, it wasn't pushy at all, I just felt very guilty for not updating you at all, I should have done that.

Also thank you so much for helping me out with the commits, the new reviews are crystal clear, will update you soon chief 🫡

[Aerozol]

My only feedback: Can we move ‘Optional:’ to the front of that second sentence? Or even just have ‘Optional:’ and get rid of everything else, since the text in the box explains it clearly enough?

[hrik2001]

Hi @MonkeyDo @Aerozol. This is what I came up with so far

1. There was dilemma over "Send Song" and "Personally Recommend", so I felt like "Send Recommendation" sounds better
2. Added a small write-up to explain the case of not having followers
3. What do you think about remove the paragraph which says "Tell your above chosen followers why are you recommending them 1985 by Freddie Gibbs, The Alchemist (Optional)" as @Aerozol suggested. I support it, but other modals have a similar sentence, hence I have kept it here for now. Should we delete from other modals too if it's decided to remove from here?

Do you think there is more room for improvement? Please do let me know

Also, sorry for the late PR, the work was mostly done way earlier but couldn't be pushed because of a very embarrassing reason. My tests were broken and to my surprise rebuilding every image and creating container wouldn't fix anything, merging master added more problem. After spending a lot of time, it came to my notice, that the build was cached. Even though the files were updated, and the images were deleted, docker still somehow used the old cache. All I had to do was add --no-cache flag and everything worked like a charm! XD

[Aerozol]

@hrik2001: Looking good, my input is to simplify stuff as much as possible:

The title ‘Recommend [song] to certain follower(s)’ is clunky. Maybe just ‘Recommend [song]’? Then in the next box have ‘Search and add followers*’, and add the asterisk (*) to the start of the last paragraph explaining why you might not be able to find the user in the drop down.

If we’re keeping the ‘tell your chosen…’ line let’s just have something like: ‘Add a message (optional)’.

For the last paragraph maybe something like: *Can’t find a user? Make sure they are following you, and then refresh the page.

[hrik2001]

@Aerozol fair point. You are letting the UI and the UX do the job for explaining to the user rather than being explicit. I will update the UI and show it here and then in the IRC too.

@MonkeyDo do you think explaining that the user should go to the listens page and then click the button next to it is necessary, or do you think it's fairly understood

[MonkeyDo]

Nice job ! Looking better and better.

I agree with everything aerozol said, including the shortened help text at the bottom. The UI is likely to change dramatically soon, and I can guarantee we'll forget to update this bit of text if the process changes :) If users can't figure how to follow another user, that's another (big) problem that shouldn't be your concern at this point.

For the modal title, maybe "Send [song] to friends" or "Share [song]" ?

other modals have a similar sentence […] Should we delete from other modals too if it's decided to remove from here? That's not necessary in this PR at least. I don't think this is a required style of how we do modals, and other modals might need the extra helping text — but then again perhaps they don't and we can revisit that. I think it's only the pin recording modal? The CB review one doesn't have one.

[hrik2001]

Oops, I had accidentally changed it to recordingMSID

Also, I wanted to ask you about the parameter one, when NamePill calls the function, it can't call with any parameter (apart from mouse event). In the modal code, the bind method creates a function, that wraps an existing function with a parameter, which is how the parameter is passed to the NamePill component.

So should I write a test to see if the function is called with a mouse event or not?

[MonkeyDo]

Ah, you're right my comment was a generic one, but it won't be of any this in this particular situation, so let's leave it like it is. :)

[hrik2001]

Hi, I believe everything has been done, please feel free to leave feedbacks. :)

[hrik2001]

@MonkeyDo @Aerozol Opinions?

[MonkeyDo]

Starting to look great ! I think the "search and add followers*" text should go above the user selection component, that'll make more sense. Apart from that I'm happy to merge, deploy and get a feel for it.

[Aerozol]

Nice! Yup, my suggestion was the ‘search and add followers*’ to go in the search box (actually could just be ‘search followers*’ now that I think about it)… And then ‘Add a message (optional)’ in the space under it

[MonkeyDo]

Reporting comments from atj on the IRC channel:

Normal people don't know what a "modal" is. I suggest you change "and then reopen this modal" to "and then try again". Or if you are wedded to the wording use "dialog" instead.

Also, "Search and add followers" is confusing to me. I'd just go with "Add followers".

[MonkeyDo]

Awesome work @hrik2001 ! So happy to see this feature out !