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

Created a translator for Antikvarium.hu #1411

Merged
merged 8 commits into from Nov 11, 2017

Conversation

Projects
None yet
5 participants
@petervelosy
Copy link
Contributor

petervelosy commented Sep 14, 2017

No description provided.

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Sep 15, 2017

Thanks! In general this looks great, but some higher level concerns:

Thanks again!

@dstillman

This comment has been minimized.

Copy link
Member

dstillman commented Sep 15, 2017

We still have some users on IE and older Safari versions and I think we'd prefer to not break things for them by using ES6 javascript like .includes and arrow assignments (those were the ones I spoted; there may be others).

We're now pretty comfortable allowing ES6 for new development. We don't need to go out of our way to break existing translators, particularly core ones, but Safari 10, which supports most ES6 features (certainly let, arrow functions, and String.includes()), can be installed on Macs going back to 2007-2009. (The Safari connector itself now requires macOS 10.11.) iOS 10, which included Safari 10, installs back to the iPhone 5 from five years ago. And I don't think we need to make any development decisions based on the bookmarklet in IE.

@zuphilip

This comment has been minimized.

Copy link
Collaborator

zuphilip commented Sep 15, 2017

Additionally, please also add some test cases.

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Sep 15, 2017

OK, thanks Dan. @petervelosy ignore the part about ES6 then, but the other issues do still apply, as does the request for test cases per @zuphilip

petervelosy and others added some commits Sep 16, 2017

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Oct 1, 2017

Please check if this all makes sense. We'd want the extra information in a note, not the Extra field, so I moved that. Apart from that I just fixed things and added multiples & tests (please do test your code before comitting, though; what you had didn't run at all)

@AronSeidl

This comment has been minimized.

Copy link

AronSeidl commented Nov 10, 2017

Thank you for all these changes and the clarification, it all makes sense now. Is there anything else I need to do, or is this translator ready for merging?

@petervelosy

This comment has been minimized.

Copy link
Contributor Author

petervelosy commented Nov 10, 2017

P.s. The last comment was by me, but a colleague was logged in with his GitHub account on this PC.

@zuphilip zuphilip merged commit 250985d into zotero:master Nov 11, 2017

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 Nov 11, 2017

This PR is now merged and the new translator will get deployed now 🚀 .

Thank you very much @petervelosy!

psisquared2 added a commit to psisquared2/translators that referenced this pull request Feb 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.