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

Feature/lyrics #247

Merged
merged 5 commits into from Feb 11, 2019

Conversation

Projects
None yet
2 participants
@trekiteasy
Copy link
Collaborator

trekiteasy commented Feb 8, 2019

A Lyrics search feature : when a track is playing, we can go to the Lyrics page (from the left menu) to get the lyrics.

@nukeop

This comment has been minimized.

Copy link
Owner

nukeop commented on 2734f9e Feb 5, 2019

why not use fetch instead of request-promise?

This comment has been minimized.

Copy link
Collaborator Author

trekiteasy replied Feb 5, 2019

You are probably right, fetch should work here.
This branch is not ready for prime yet for sure ! It's a work in progress. (Even thought the Lyrics.index.js search function does perform a search with one source)... but I'm not sure how this feature should be added to the UI so it stays nice and uncluttered.

This comment has been minimized.

Copy link
Owner

nukeop replied Feb 5, 2019

Years ago Spotify had an 'apps' feature that let users install third party plugins. The stock menu had an apps browser, and you could add any number of them to your client. Then, each plugin would appear as an entry on the left with its own icon etc. After clicking that, the plugin could display its own page with whatever content.

Menu entries visible in this screenshot:
https://www.poderpda.com/wp-content/uploads/2015/02/spotify.png

Something like that is also available on Deezer:
https://conteudo.imguol.com.br/c/noticias/2014/12/18/recurso-lyrics-do-deezer-permite-que-usuario-escute-musica-e-acompanhe-a-letra-1418926724163_615x470.jpg

Added a working lyrics feature integrated to the UI
- Added a lyrics menu entry on the left sidebar
- The lyrics are loaded after the stream is loaded
- It tries to get the lyrics from the fastest out of 3 sources
- I'm not sure about the icon

@nukeop nukeop added the under review label Feb 8, 2019

@nukeop

This comment has been minimized.

Copy link
Owner

nukeop commented Feb 9, 2019

Would be good if we were able to check if the lyrics actually exist, it seems that a http result 200 is not enough. Maybe there's a package that searches for lyrics with fallbacks? If not, this would be a good candidate to go into nuclear-core, or its own separate package (like I did with the Pitchfork best new music scraper).

I'm getting some nonsense as lyrics sometimes:

2019-02-09-020212_722x230_scrot

<div className={styles.lyrics_header_overlay}>
<div className={styles.lyrics_header_container}>
<div className={styles.lyrics_artist_name_container} >
<h1>{track.name} by {track.artist}</h1>

This comment has been minimized.

@nukeop

nukeop Feb 9, 2019

Owner

Better to use the Header component, for uniform style across all views, instead of a plain

@nukeop nukeop added needs changes and removed under review labels Feb 9, 2019

@trekiteasy

This comment has been minimized.

Copy link
Collaborator Author

trekiteasy commented Feb 11, 2019

You are right :

  • I spinned-off the lyric part to an external package (simple-get-lyrics).
  • there were already some fallbacks to look on different sources, but I added some filtering to avoid the "Unfortunately..." message
  • I used the Header component for the title (and changed the line-height to better display multiline titles)
@nukeop

This comment has been minimized.

Copy link
Owner

nukeop commented Feb 11, 2019

Fantastic, great work.

@nukeop nukeop merged commit 87b7a19 into master Feb 11, 2019

2 of 3 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment