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 translator for svd.se #1690

Merged
merged 4 commits into from Sep 6, 2018

Conversation

@sebastian-berlin-wmse
Copy link
Contributor

commented Jul 6, 2018

This is the website for the Swedish newspaper Svenska Dagbladet.

Add translator for svd.se
This is the website for the Swedish newspaper Svenska Dagbladet.
Fix translator for svd.se according to code review
* Updated target regex.
* Added checks to avoid errors when values weren't found.
@sebastian-berlin-wmse

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2018

I've updated according to the comments in #1691. Let me know if there's anything else.

@adam3smith
Copy link
Collaborator

left a comment

Thanks! Some small requests.

"abstractNote": "Nya Karolinska-skandalen har blivit en nationell fråga som riskerar att skada Sveriges rykte som innovationsland. Det säger närings- och innovationsminister Mikael Damberg (S) till SvD.",
"language": "sv",
"libraryCatalog": "www.svd.se",
"publicationTitle": "SvD.se",

This comment has been minimized.

Copy link
@adam3smith

adam3smith Aug 22, 2018

Collaborator

Svenska Dagbladet would be better here, no?

This comment has been minimized.

Copy link
@sebastian-berlin-wmse

sebastian-berlin-wmse Aug 31, 2018

Author Contributor

Yes, I think so.

"title": "Snapshot"
}
],
"tags": [],

This comment has been minimized.

Copy link
@adam3smith

adam3smith Aug 22, 2018

Collaborator

You should be able to add tags using doc.querySelectorAll("#ArticleTags-tag-link") and then cycle through the result, textContent and push to tags.

This comment has been minimized.

Copy link
@sebastian-berlin-wmse

sebastian-berlin-wmse Aug 31, 2018

Author Contributor

Added code and tests for tags.

item.creators = [];
// The author string is in the format:
// FirstName LastName | Email
var authorString = attr(doc, 'meta[name="author"]', "content");

This comment has been minimized.

Copy link
@adam3smith

adam3smith Aug 22, 2018

Collaborator

again, are we sure there are never multiple? Otherwise better to querySelectorAll and loop through results.

This comment has been minimized.

Copy link
@sebastian-berlin-wmse

sebastian-berlin-wmse Aug 31, 2018

Author Contributor

I haven't come across any case where there were multiple authors when developing this translator. If an article like that is discovered, I'm happy to add functionality for handling that, but I don't want to guess what it would look like.

"translatorID": "7adba17c-7e98-4a13-8325-e19383b09eab",
"label": "Svenska Dagbladet",
"creator": "Sebastian Berlin",
"target": "^https://www\\.svd\\.se/(sok\\?)?",

This comment has been minimized.

Copy link
@adam3smith

adam3smith Aug 22, 2018

Collaborator

if the last element has a ?, you don't need it. I'd just use ^https://www\\.svd\\.se/., i.e. every page on the site other than the homepage.

This comment has been minimized.

Copy link
@sebastian-berlin-wmse

sebastian-berlin-wmse Aug 31, 2018

Author Contributor

True, removed that.

Fix review comments
* Removed unnecessary part from target regex.
* Changed `publicationTitle` to "Svenska Dagbladet".
* Added functionality for adding tags.

@adam3smith adam3smith merged commit 9d070c3 into zotero:master Sep 6, 2018

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2018

Great, thanks!

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