isawnyu / isaw.web

Isaw website buildout
http://isaw.nyu.edu
1 stars 3 forks source link

AWLDjs links not producing popups #197

Closed paregorios closed 6 years ago

paregorios commented 7 years ago

_Reviewing scrumdo story 91:_

I am able to create an appropriate <div> in a body text field with the lightbulb icon in tinymce, but when I view the base view I do not see expected awldjs behaviors. For example:

https://isaw4-dev.atlantides.org/members/paregorian/fun-with-publications-2/amheida-i-ostraka-from-trimithis-1

alecpm commented 7 years ago

It looks like AWLD.js doesn't like https links. Changing those two links to http caused the overlay to appear (though in the Wikipedia case, the overlay is empty, which is what I've been seeing everywhere). I will look into updating awld.js to support both http and https urls.

paregorios commented 7 years ago

That's surprising to me for wikipedia and pleiades. But see what you find.

alecpm commented 7 years ago

AWLD's registry of supported url formats (https://github.com/isawnyu/awld-js/blob/master/registry.js) hardcodes http for all services. I can make a pull request to support urls independent of protocol, I'll either need to fork the package or be given commit rights. Not sure what's up with wikipedia, still looking into that one.

alecpm commented 7 years ago

I've pushed a fix to staging which modifies the AWLD.js libs to accept HTTPS urls, and also to fetch data over secure connections whenever possible so that the data loading does not fail when requesting an HTTP resource on an HTTPS page on isaw.nyu.edu. For those services that do not support SSL (or provide invalid SSL certs) I have added a flag indicating such so that AWLD will use YQL to proxy the request via HTTPS when it needs to request an HTTP resource in the context of an HTTPS page. Changeset is here: https://github.com/isawnyu/isaw.web/commit/9ef55a3ce37605a961615a0d2b7f982f5daa9617

I found one other issue in AWLD.js which cropped up when testing. The Google staticmap API it uses to fetch images of a location has a very limit on the number of requests that can be made to the API without a key. The limit appears to be imposed on a per requesting IP basis, so it will only appear on the browser side after a client has visited a number of pages making use of static map images in AWLS.js overlays. Once the limit is hit the client will see a broken image. It would make sense to include a key in the static map requests, but it might not make sense to provide the key hardcoded in the library. Ideally, there would be an argument to awld.init for passing in an maps API key for a specific site.

alecpm commented 7 years ago

If things look good to you, I'll make a pull request into AWLD.js master with the above changes. If you'd like me to pursue the map API key issue I can make another pull request for that.

paregorios commented 7 years ago

@alecpm yes, this looks good. Please:

Let's defer chasing the map API key issue for now.

paregorios commented 7 years ago

@alecpm and @skleinfeldt ... do you know if this was actually deployed or not? Thanks.

alecpm commented 7 years ago

Yes, this was deployed some time ago, but a migration step was missed that prevented it from working. The image viewer issue #202 required running the migration step, so this should be fully deployed now.