Adds the ability to change the default base URL for the images of IA BookReader #420
Conversation
|
@roromedia thanks! could you please remove the two dot files from this pull? I will test in the meantime and give you feedback if some changes are needed (If at all). Thanks!!! |
|
I will also change the base branch to 1.4.0. 1.3.0 is closed by now. |
There was a problem hiding this comment.
@roromedia I added a few comments. You can simply the code and add a bit of more validation/checks for existence in your pull still. Also, there is one entry missing from your pull which is the addition of a new config entry and a default after this line
format_strawberryfield/config/schema/format_strawberryfield.schema.yml
Lines 320 to 322 in cf23b58
let me know if you have any questions. Almost there! Thanks!
| @@ -127,6 +127,7 @@ public static function defaultSettings() { | |||
| 'manifesturl_source' => 'iiifmanifest', | |||
| 'max_width' => 720, | |||
| 'max_height' => 480, | |||
| 'ia_reader_images_base_url' => '', | |||
There was a problem hiding this comment.
Formatters using the ::getSetting method will eventually call this one here
see
// Merge defaults if we have no value for the key.
if (!$this->defaultSettingsMerged && !array_key_exists($key, $this->settings)) {
$this->mergeDefaults();
}
return $this->settings[$key] ?? NULL;
}
Which means by setting the return of getIABookReaderImagesDefaultBaseURL() directly you can save yourself that function and you can also call the defaultSettings method directly https://github.com/esmero/format_strawberryfield/pull/420/files#diff-b23e313798ac690e86b6697e23a4b490f091ea3b026b9480def3634656daf722R217 there instead of the private function
There was a problem hiding this comment.
I added the default url to the format_strawberryfield.schema.yml but I wasn't able to reuse or access it as the default value in the defaultSettings() function so I hardcoded the url there again.
js/iiif-iabookreader_strawberry.js
Outdated
| @@ -40,7 +40,7 @@ | |||
| iiifmanifest: strawberrySettings['manifest'], | |||
| iiifdefaultsequence: null, //If null given will use the first sequence found. | |||
| maxWidth: 800, | |||
| imagesBaseURL: 'https://cdn.jsdelivr.net/gh/internetarchive/[email protected]/BookReader/images/', | |||
| imagesBaseURL: strawberrySettings['iareaderimagesbaseurl'], | |||
There was a problem hiding this comment.
This JS here needs to check if strawberrySettings['iareaderimagesbaseurl'] is defined (typeof !== "undefined")
before using it directly. You (sadly) will have to hardcode also (as a fallback) the https://cdn.jsdelivr.net/gh/internetarchive/[email protected]/BookReader/images/' string here. Why? Because after updating the code many other Archipelago users will have still their Display Modes Cached, but the JS will update immediately which means that new variable will not be passed at all until someone either clears caches (expensive on a large repo) or save the formatter settings manually
| @@ -407,6 +422,7 @@ public function processElementforMetadatadisplays($delta, array $jsondata, Field | |||
| $max_width = $this->getSetting('max_width'); | |||
| $max_width_css = empty($max_width) || $max_width == 0 ? '100%' : $max_width .'px'; | |||
| $max_height = $this->getSetting('max_height'); | |||
| $ia_reader_images_base_url = $this->getSetting('ia_reader_images_base_url'); | |||
There was a problem hiding this comment.
The logic on 'iareaderimagesbaseurl' => strlen($ia_reader_images_base_url) > 0 ? $ia_reader_images_base_url : $this->getIABookReaderImagesDefaultBaseURL(),
Should go here. And you can also check/add/strip/restore the last "/" to be sure it always ends in a single "/" in the same line.
… to missing text-transform capability
| @@ -227,7 +227,7 @@ BookReader.prototype.buildToolbarElement = (function (super_) { | |||
| var $el = super_.call(this); | |||
| var readIcon = ''; | |||
| $el.find('.BRtoolbarRight').append("<span class='BRtoolbarSection Islandora'>" | |||
| + "<button class='BRpill zoomPage js-tooltip' title='Fine zoom'>ZOOM</button>" | |||
| + "<button class='BRpill zoomPage js-tooltip text-uppercase' title='Fine zoom'>Zoom</button>" | |||
|
@roromedia thanks for addressing the issues. Because of the spacing changes (which are good) I will have to actually git merge this against my local 1.4.0 to be able to see if there are any hidden issues. So please give me until tomorrow noon (EST) to have this properly tested. Thanks again |
There was a problem hiding this comment.
@roromedia approving this. Since I have this larger pull open #419 that touches many parts of the same code (and extensively) I will have to modify your code too before merging mine, so please expect a larger moving around of your contributed code during the next few days (will try to preserve as much as I can). Please make sure you do yet again a pull against 1.4.0 when that happens. Thanks
This is necessary as because of GDPR as it is not allowed to load data from external sources without consent of the user. Now by making the path configurable the user is able to self-host these images.
@DiegoPino : There is for sure a more standardised way to put the default configuration path. I now added a function which returns this path but I guess it would be better retrieved out of a settings-file or via \Drupal::config()?