Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upPubMedLabs update to PubMed.js #1801
Conversation
Dec 19, 2018
added some commits
CheViana
reviewed
Dec 20, 2018
PubMed.js Outdated
CheViana
reviewed
Dec 20, 2018
PubMed.js Outdated
CheViana
reviewed
Dec 20, 2018
PubMed.js Outdated
CheViana
reviewed
Dec 20, 2018
PubMed.js Outdated
Dec 20, 2018
added some commits
This comment has been minimized.
This comment has been minimized.
@zuphilip Hi Phillip, thanks for writing the PubMed.js. I'm trying to fix some user errors noticed on the new Pub Med Labs site. Namely, abstract pages are not being recognizes as "journalArticle" types. Are the commented out code in the doWeb function still needed? |
This comment has been minimized.
This comment has been minimized.
Almost certainly not, no. These have been there for a couple of years and no one has missed them. Feel free to remove during re-write. |
akinhwan
changed the title
[WIP] PubMedLabs update to PubMed.js
PubMedLabs update to PubMed.js
Dec 24, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@adam3smith could you take a look at this PR when you get a chance? I made sure all tests are passing in Scaffold. Let me know if there is anything you'd like me to address! Thanks |
akinhwan
and others
added some commits
Jan 2, 2019
This comment has been minimized.
This comment has been minimized.
CheViana
commented
Jan 2, 2019
Could somebody with write access to repo please merge this? @adam3smith @zuphilip |
This comment has been minimized.
This comment has been minimized.
We'll need to review before merging, but as we indicated at @akinhwan ' s post above, are aware of this and will do asap. |
akinhwan
referenced this pull request
Jan 15, 2019
Closed
Zotero Item Selector unchecked selections #1804
adam3smith
added some commits
Feb 2, 2019
This comment has been minimized.
This comment has been minimized.
@zuphilip -- do you have time to give this a quick look. I ran all automated tests without problems. |
zuphilip
reviewed
Feb 3, 2019
This looks good. I have fixed two other warnings in the old code and added some comments. I have two more questions for you @adam3smith . |
if (!main) return; | ||
|
||
var booksections = doc.getElementById('book-sections'); | ||
if (!main && !booksections) return; |
This comment has been minimized.
This comment has been minimized.
zuphilip
Feb 3, 2019
Collaborator
I don't understand the logic here which you just changed recently. It is possible that main
does not exist but booksections
does, then this would continue (not both conditions fulfilled) and the next line would fail. I.e. main
has to exist for the next line, but then we can change back to the original condition, because this will not change anything else.
This comment has been minimized.
This comment has been minimized.
adam3smith
Feb 4, 2019
Collaborator
Good catch. I'm not sure there are cases where just booksections
appears, but in any case we're not using them, so I've removed booksections
here entirely and make sure we don't expect it in detectWeb
.
It might be that there are book sections without UID and for those this translator won't detect (or import) now -- if we find one in the future, we can simply add logic in this function.
Given that the parseFromItemprop
function only covers books, I don't believe anyone has every come across an example.
title: ZU.trimInternal(title), | ||
checked: checkbox && checkbox.checked | ||
}; | ||
if (results.length > 0){ |
This comment has been minimized.
This comment has been minimized.
zuphilip
Feb 3, 2019
•
Collaborator
Isn't it enough to either do the test on line 146 and fail when length is zero or do the condition here such that the following lines are only called when the length is positive?
zuphilip
merged commit 5772369
into
zotero:master
Feb 4, 2019
1 check passed
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
@zuphilip @adam3smith thank You so how would I know when it is live? Would it be in the next release? https://www.zotero.org/support/changelog Thanks :) |
This comment has been minimized.
This comment has been minimized.
Translators update daily, independent of releases, so this has been live for >a week. |
akinhwan commentedDec 19, 2018
•
edited
updated getUID and getSearchResults functions, added a testCase for PubMedLabs