MBS-12205: Add "play on ListenBrainz" button

[reosarevok]

Implement MBS-12205

As requested, this mostly copies the functionality of the existing script (https://gist.github.com/MonkeyDo/6b919302e97dc979f8277b9c71ff82ab). It only shows on overview pages for collections and RGs, but that's also true of the script so hopefully not a problem. Ideally, LB would support passing an RG or collection MBID directly.

Examples:

[MonkeyDo]

Yes, I do agree we should reconstruct a playlist based on a collection from the database directly.
Since this wasn't something I could do with the script I didn't go there, but it can certainly be improved upon.

I also think the tags and genres pages would be good candidates for a similar mechanism (reconstruct a playlist by fetching recordings with tag = x)

[reosarevok]

@Aerozol: if you have some time to look into this, I'd be grateful - I'm not super convinced by the position of the button on stuff without cover art (so, the recording example above, or even just releases/RGs without any art).

[Aerozol]

@reosarevok Hmm, the placement seems fine to me, if we really want to make it stand out (since nothing else gets such a big button, and right at the top!)
Only change is that I think we can remove the LB logo since we already say ‘Play on ListenBrainz’. A ‘play’ icon might be more tempting for people to click it:

Or even just go with the play button, since I don’t think where it gets played is much of a factor (until LB gets known). Just not sure about there being no signposting that it links to a new site:

[reosarevok]

the placement seems fine to me, if we really want to make it stand out

Yeah, my worry was that it seemed a bit too overpowering :) But it might be fine.

[MonkeyDo]

It is indeed a good question, do we want it to stand out that much?
I initially did it that way because, well, it was mostly for myself, but is there going to be community backlash?
I think aerozol's mockup with the more subdued button with no colorful logo would help in that respect.

Here's a couple of less prominent alternatives, not sure any of them have much merit but thought I'd try them out just to be exhaustive:

As a tab: (IMO that breaks what users expect of a tab)

Lower in the sidebar:

As text rather than a button:

Below the tracklist:

Same but to the right:

Floaty button on the right of the header (straight away no for this one)

[Pardon the french]

[Aerozol]

Ooh, so exhaustive!
Now my pick would be the bottom right below the tracklist, but with a play icon instead of the LB icon.

FYI I have addressed this in two possible ways in the redesign:

Top right

And when clicking on the album art column (so you can play any release from an artist page/table)

We could make a new type of button/something fancy here too but I think it’s a good idea to stick with the current button styles for the current site.

[reosarevok]

Keep in mind this is also in recording, RG and some collection pages :) I guess whatever we pick should be consistent among those, ideally

[mwiencek]

Nitpick: the incompatible-type isn't really a Flow issue, since it has no way of knowing these are recordings. I'm not sure why it's giving prop-missing there, that does seem like a Flow issue. But I was able to work around both by casting the list to $ReadOnlyArray<CoreEntityT> first:

Suggested change

	// $FlowIssue (both incompatible-type and prop-missing, sigh)
	? entities.map((entity: RecordingT) => entity.gid)
	? (entities: $ReadOnlyArray<CoreEntityT>).map(entity => entity.gid)

[yvanzo]

I don’t mind either way for now because it’s just a global issue in MBS JS code.

Just as a side-note: CoreEntityT is overused in general. It is better than using any but still. This is one of the main issues I encountered while working on #2732 the 4 first commits of which are just about replacing a few unneeded CoreEntityTypeT occurrences. There are 300+ occurrences in the JS code, there are good chances that many of these are not fine-grained enough, later on generating the kind of Flow issue that you have here.

[reosarevok]

But it already knows that entities: $ReadOnlyArray<T> and T: CoreEntityT - as such, it should know these have gids. In any case, casting to CoreEntityT again seems like a hack. What would be ideal is for Flow to understand that if we know collectionEntityType is 'recording' then we also know that entities is $ReadOnlyArray<RecordingT>, but I'm guessing with the way we're doing this now, we can't just use the https://flow.org/en/docs/types/unions/#toc-disjoint-object-unions thing since hardcoding the collectionEntityType directly for all, but typing it as T['entityType']. I guess this might work better if the type was a proper union instead?

[mwiencek]

Actually yeah, I missed that props is a disjoint object union, so we could use that:

  const recordingMbids = props.collectionEntityType === 'recording' && props.entities.length > 0
    ? props.entities.map(entity => entity.gid)
    : null;

[reosarevok]

I had to change the type declaration a bit, but now it's happy (we can squash on merge as needed).

[yvanzo]

The code (typing apart) looks good 👍 but the commit message is very incomplete.

I did successfully test it locally 👍 using the production standby database and visiting some random pages as no example is given in PR’s description.

There is no request to ListenBrainz so no overhead 👍 however ListenBrainz might not be able to play it as user would expect it from the button’s label. Not a showstopper, just to be known and documented.

The placement is very nice 👍 as it clearly and very visibly promotes ListenBrainz without getting into the way.

The icon has not been changed despite @Aerozol’s comment. Probably not a showstopper either, it can be changed later on if there is some hurry about this ticket.

[yvanzo]

In reply to @Aerozol’s and @MonkeyDo’s comments on placement and community acceptance:

I don’t think that there could be any community backslash about it since the link is clearly labeled and put in the sidebar where prominent external links (including CritiqueBrainz) are expected. The current placement (prominently at the top just under the cover art) is very relevant as it is a top-notch MeB project. It seems to be a good balance. Good work!

However it’s always useful to communicate more about it with the community if you want this new feature to be noticed at the very least.

[reosarevok]

Since the intent of the whole thing is, let's be honest, more promoting LB than giving a place to play music (both, but in that order), I think the logo probably makes sense? Although I'm fine with changing it to a play button if we prefer a bit later on :)

[yvanzo]

Thanks for having improved the commit message. 🚢

Please check with @bitmap about the Flow suggestion he made. Whether your change is enough or if it should be changed later on in master branch. (It doesn’t make a difference for now in production.)

[yvanzo]

In reply to @Aerozol about the icon:

As mentioned in a later discussion on #metabrainz, the added button is more about promoting LB than promising that it will play anything. So using the logo makes sense. The button’s label is a trick but I don’t have any better suggestion.

[yvanzo]

@brainzbot, please retest it.

[Aerozol]

@yvanzo re. promoting LB, the icon doesn’t invite people to click the button - the play button does, so I think that would ‘promote’ LB more.
Unless you already know the LB logo/colours the icon is not that helpful. I would consider taking it out completely tbh, that the button has ListenBrainz clearly on there which is pretty cool.
But none of this is a big issue, just digging into it : )

[reosarevok]

@Aerozol I think I'd feel a bit bad making it a play button because I feel that would make it even more frustrating if they click and they cannot actually play anything :/

[Aerozol]

@reosarevok but… it says ‘Play on ListenBrainz’?
But mainly, generally speaking I’m not a fan of adding extra bits in unless it’s justified/there’s a reason, and I’m not sure the LB icon meets that criteria.

Regardless thanks for putting this in, great feature!

[reosarevok]

I know, I know. But having the play icon and the word Play seems like it's stronger? Dunno. We can change it in January if you prefer, no worries :) (although to be fair, the play button is also new for MB itself :) )

[yvanzo]

@Aerozol wrote:

But mainly, generally speaking I’m not a fan of adding extra bits in unless it’s justified/there’s a reason, and I’m not sure the LB icon meets that criteria.

ListenBrainz's icon is added just like other external websites' icons have been added before.
Maybe it should be placed under external-favicons/ instead of icons/ to make it more explicit?

@Aerozol wrote:

@reosarevok but… it says ‘Play on ListenBrainz’?

As mentioned in a previous comment which links to IRC, this button is more about promoting ListenBrainz than necessarily playing something. We can come with a more accurate (but still incentive) label such as “Check it out on ListenBrainz”. ListenBrainz should also more gracefully handle when it fails at playing anything.

[Aerozol]

@yvanzo The button format is quite different to the format of the other external links - can you explain why it’s helpful to have the LB icon there? Personally I don’t think more noise and imagery makes it easier for people to navigate/understand our already-messy sidebar.

I wouldn’t change the name from ‘Play on ListenBrainz’, that’s currently the primary function even if it doesn’t always work.
Edit: Instead of primary function I should have said ‘call to action’. I can’t think of any other reason why somebody would go to LB from MB. Being able to play the song (or try to) is a massive new function!