Skip to content
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

Add MCV.js #1687

Merged
merged 5 commits into from Jul 8, 2018

Conversation

Projects
None yet
3 participants
@owcz
Copy link
Contributor

commented Jul 5, 2018

Translator for https://www.mcvuk.com

@adam3smith
Copy link
Collaborator

left a comment

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.

MCV.js Outdated
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.

Copy link
@adam3smith

adam3smith Jul 8, 2018

Collaborator

use string.includes() for string testing & take advantage of getSearchResults

MCV.js Outdated
return "blogPost";
} else if (url.indexOf("/search?query=") != -1) {
return "multiple";
} else return null;

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jul 8, 2018

Collaborator

no need for the else; that's implicit.

MCV.js Outdated
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.

Copy link
@adam3smith

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.

Copy link
@owcz

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

@owcz

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2018

good point re: type
I didn't know if all or just some of their content was also in their magazine
but changed the type anyway

rmv else null
thought i already did this
@adam3smith
Copy link
Collaborator

left a comment

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.

MCV.js Outdated

function detectWeb(doc, url) {
if (/\/(business|development|esports|influence)\//.test(url)) {
return "blogPost";

This comment has been minimized.

Copy link
@adam3smith

adam3smith Jul 8, 2018

Collaborator

update the item type here and in doWeb and update tests

MCV.js Outdated
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.

Copy link
@adam3smith

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.

@adam3smith adam3smith merged commit c001136 into zotero:master Jul 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@owcz owcz deleted the owcz:mcv branch Jul 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.