-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fetch image from Wikimedia via MusicBrainz #49
Conversation
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.
Thank you for this Pull-Request, this is awesome! I've marked most of HoundCI's suggestions as resolved since they should be outdated now that #50 is merged.
Can you apply the suggestions and rebase on top of main? 🙏
Hey @dmcneary, any chance you will have some time to look over the requested changes? |
lib/sonice/public/script.js
Outdated
const filename = | ||
imageUrl.substring(imageUrl.lastIndexOf('/') + 1); | ||
imageUrl = | ||
'https://commons.wikimedia.org/wiki/Special:Redirect/file/' + |
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.
Line is too long.
lib/sonice/public/script.js
Outdated
let commonsStub = | ||
'https://commons.wikimedia.org/wiki/File:'; | ||
if (imageUrl.startsWith(commonsStub)) { | ||
const filename = |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
lib/sonice/public/script.js
Outdated
const imagesArr = []; | ||
for (let i = 0; i < relations.length; i++) { | ||
if (relations[i].type === 'image') { | ||
let imageUrl = relations[i].url.resource; |
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.
'let' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
lib/sonice/public/script.js
Outdated
if (!artist) return callback(); | ||
cache = artistImage.cache; | ||
artist = encodeURI(artist); | ||
const getImageFromCache = |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
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 think that merging the main
branch should make these hints disappear.
lib/sonice/public/script.js
Outdated
var changeBackground = function(){ | ||
artistImage(currentSong.artist, function(url) { | ||
$('body').background(url) | ||
}) |
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.
Missing semicolon.
lib/sonice/public/script.js
Outdated
// Change background on the body regularly | ||
var changeBackground = function(){ | ||
artistImage(currentSong.artist, function(url) { | ||
$('body').background(url) |
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.
Missing semicolon.
lib/sonice/public/script.js
Outdated
} | ||
|
||
// Change background on the body regularly | ||
var changeBackground = function(){ |
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.
'changeBackground' was used before it was defined.
lib/sonice/public/script.js
Outdated
@@ -52,6 +122,16 @@ $(function() { | |||
|
|||
if (artistChange || songChange) | |||
$('#vote').removeAttr('disabled').show() | |||
|
|||
if (artistChange) | |||
changeBackground() |
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.
Expected '{' and instead saw 'changeBackground'.
Missing semicolon.
lib/sonice/public/script.js
Outdated
const filename = | ||
imageUrl.substring(imageUrl.lastIndexOf('/') + 1); | ||
imageUrl = | ||
'https://commons.wikimedia.org/wiki/Special:Redirect/file/' + |
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.
Line is too long.
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.
Perhaps this can be in a variable nearer the top of the file, such as wikimediaUrlPrefix
?
lib/sonice/public/script.js
Outdated
for (let i = 0; i < relations.length; i++) { | ||
if (relations[i].type === 'image') { | ||
let imageUrl = relations[i].url.resource; | ||
let commonsStub = |
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.
'let' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
Hey @sunny, Been busy with finals. I refactored your suggestions, still some little complaints from CI but let me know what you think. |
lib/sonice/public/script.js
Outdated
if (!artist) return callback(); | ||
cache = artistImage.cache; | ||
artist = encodeURI(artist); | ||
const getImageFromCache = |
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 think that merging the main
branch should make these hints disappear.
lib/sonice/public/script.js
Outdated
const filename = | ||
imageUrl.substring(imageUrl.lastIndexOf('/') + 1); | ||
imageUrl = | ||
'https://commons.wikimedia.org/wiki/Special:Redirect/file/' + |
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.
Perhaps this can be in a variable nearer the top of the file, such as wikimediaUrlPrefix
?
} | ||
|
||
// Change background on the body regularly | ||
var changeBackground = function () { |
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.
'changeBackground' was used before it was defined.
$('#vote').removeAttr('disabled').show() | ||
$('#vote').removeAttr('disabled').show(); | ||
|
||
if (artistChange) changeBackground(); |
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.
Expected '{' and instead saw 'changeBackground'.
|
||
if (artistChange || songChange) | ||
$('#vote').removeAttr('disabled').show() | ||
$('#vote').removeAttr('disabled').show(); |
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.
Expected '{' and instead saw '$'.
else | ||
$('title').text(artist + (artist && title ? ' — ' : '') + title) | ||
$('title').text(artist + (artist && title ? ' — ' : '') + title); |
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.
Expected '{' and instead saw '$'.
|
||
if (!title) | ||
$('title').text('So nice') | ||
$('title').text('So nice'); |
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.
Expected '{' and instead saw '$'.
.then(res => res.json()) | ||
.then(data => { | ||
const relations = data.relations; | ||
const commonsStub = |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
fetch(url) | ||
.then(res => res.json()) | ||
.then(data => { | ||
const relations = data.relations; |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
'?inc=url-rels&fmt=json'; | ||
fetch(url) | ||
.then(res => res.json()) | ||
.then(data => { |
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.
'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
mbid + | ||
'?inc=url-rels&fmt=json'; | ||
fetch(url) | ||
.then(res => res.json()) |
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.
'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').
// (https://github.com/hugovk/now-playing-radiator) | ||
const mbid = response.artist.mbid; | ||
if (!mbid) return; | ||
const url = |
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.
'const' is available in ES6 (use 'esversion: 6') or Mozilla JS extensions (use moz).
Don't know why houndci-bot is still complaining. Looks good to me! Great work, @dmcneary 🎉 |
resolves #44
(mostly)
I set up a fetch call to MusicBrainz, which I have working most of the time to grab an image and change the background. It scrapes an image from Wikimedia, as long as there is one available. Some bands, however, don't have any images on Wikimedia.