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

adjust export button xpath #1733

Merged
merged 3 commits into from Sep 8, 2018

Conversation

Projects
None yet
2 participants
@adam3smith
Copy link
Collaborator

adam3smith commented Sep 4, 2018

remove/change some tests

adam3smith added some commits Sep 2, 2018

Further fix SD
Also switch out some tests that don't make sense any more

@adam3smith adam3smith requested a review from zuphilip Sep 4, 2018

@adam3smith

This comment has been minimized.

Copy link
Collaborator Author

adam3smith commented Sep 4, 2018

well, still in my master branch, but oh well.

@zuphilip
Copy link
Collaborator

zuphilip left a comment

Some comments and questions from my side.

@@ -28,7 +28,7 @@ function detectWeb(doc, url) {
if ((url.includes("pdf") &&
!url.includes("_ob=ArticleURL") &&
!url.includes("/article/")) ||
url.search(/\/(?:journal|bookseries|book|handbooks|referenceworks)\//) !== -1) {
url.search(/\/(?:journal|bookseries|book|handbook|referenceworks)\//) !== -1) {

This comment has been minimized.

@zuphilip

zuphilip Sep 4, 2018

Collaborator

What about the referenceworks? You delete the example below and I guess they don't work the same as the other multiples, right? If so, then we should delete it here.

This comment has been minimized.

@zuphilip

zuphilip Sep 4, 2018

Collaborator

And shouldn't you adjust the changes also in the target?

Show resolved Hide resolved ScienceDirect.js
Show resolved Hide resolved ScienceDirect.js
@adam3smith

This comment has been minimized.

Copy link
Collaborator Author

adam3smith commented Sep 5, 2018

I think I addressed everything. Please take another look when you get a chance

@zuphilip zuphilip merged commit 577948f into zotero:master Sep 8, 2018

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Sep 8, 2018

Thank you @adam3smith ! You might want to clean your master branch now after the squash.

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.