-
Notifications
You must be signed in to change notification settings - Fork 774
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
New translator: FNAC books, movies, music and videogames #3378
base: master
Are you sure you want to change the base?
Conversation
item.company = value.textContent.trim(); | ||
} | ||
else { | ||
item.publisher = value.textContent.trim(); |
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.
item.publisher
should map to label
/company
without needing an explicit check.
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 don't understand, could you elaborate, please?
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.
You can just set item.publisher
regardless of item type. Zotero will put it in the correct field when saving the item.
} | ||
case 'EAN': { | ||
// Extract the EAN as ISBN if ISBN is empty | ||
item.ISBN = item.ISBN || value.textContent.trim(); |
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.
But EANs aren't ISBNs as far as I know, so we wouldn't want them in the ISBN field
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.
But EANs aren't ISBNs as far as I know, so we wouldn't want them in the ISBN field
You're right but I didn't see any "EAN" field in Zotero object. Have I missed something?
Considering both are related, I assumed it was more important to get EAN in the ISBN field, but I'm open to any suggestion.
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.
If there's no ISBN, we just don't put anything in the ISBN field. We don't save barcodes on other book catalog sites.
FNAC.js
Outdated
} | ||
case 'Nombre de pages': { | ||
// Extract the number of pages | ||
item.numPages = +value.textContent.trim(); |
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.
Don't convert to number (+
) - no point and will just lead to NaN
in the field in weird cases.
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 added a verification.
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.
No, I'm saying numPages
is a string field, so we don't need to parse the value to a number at all. It's usually fine to just put whatever is on the page into numPages
unless it needs cleaning (in which case we use a regex to remove pp.
, etc).
A FNAC translator was missing.
I used FNAC books a lot during my studies, so I thought it would be nice to offer what I coded for other people struggling with references on this website.
For non French people: it's more or less like Amazon online, although it's a physical store in the first place.