Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd MCV.js #1687
Conversation
adam3smith
requested changes
Jul 8, 2018
|
Thanks; a couple of nits, one question. Beyond that -- you're sure that blogpost is the best item type here? I don't know the publication so can't tell, it just seems that maybe treating this like a webpage or even a magazine might make more sense? But leaving this up to you. |
| function detectWeb(doc, url) { | ||
| if (/\/(business|development|esports|influence)\//.test(url)) { | ||
| return "blogPost"; | ||
| } else if (url.indexOf("/search?query=") != -1) { |
This comment has been minimized.
This comment has been minimized.
adam3smith
Jul 8, 2018
Collaborator
use string.includes() for string testing & take advantage of getSearchResults
| return "blogPost"; | ||
| } else if (url.indexOf("/search?query=") != -1) { | ||
| return "multiple"; | ||
| } else return null; |
This comment has been minimized.
This comment has been minimized.
| item.publicationTitle = "MCV"; | ||
| item.date = attr(doc,'meta[name="published"]','content'); | ||
| item.title = item.title.replace(/- MCV$/,''); | ||
| if (item.creators[0].lastName == "Editors") { |
This comment has been minimized.
This comment has been minimized.
adam3smith
Jul 8, 2018
Collaborator
just to make sure -- this would always be the first author? No need to cycle through creatores?
This comment has been minimized.
This comment has been minimized.
owcz
Jul 8, 2018
Author
Contributor
yep this is just so the generic "MCV Editors" byline doesn't get digested into "Editors, MCV"
don't believe I've seen two+ authors on this site, but the generic byline wouldn't be used with a second author, fwiw
owcz
added some commits
Jul 8, 2018
This comment has been minimized.
This comment has been minimized.
|
good point re: type |
adam3smith
reviewed
Jul 8, 2018
|
You didn't update the type everywhere and then need to update tests (also e.g. with the language capitalization), but this looks ready otherwise. |
|
|
||
| function detectWeb(doc, url) { | ||
| if (/\/(business|development|esports|influence)\//.test(url)) { | ||
| return "blogPost"; |
This comment has been minimized.
This comment has been minimized.
| item.ISSN = "1469-4832"; | ||
| item.date = attr(doc,'meta[name="published"]','content'); | ||
| item.title = item.title.replace(/- MCV$/,''); | ||
| item.language = item.language.replace('us','US'); |
This comment has been minimized.
This comment has been minimized.
adam3smith
Jul 8, 2018
Collaborator
just in case let's wrap this into an if (item.language) so it doesn't break things if they remove language or it's missing.
owcz commentedJul 5, 2018
Translator for https://www.mcvuk.com