Commit 68be70df authored by Robert Knight's avatar Robert Knight Committed by Nick Stenning

Make boot script load settings from correct config tags (#243)

An inconsistency crept in where the annotator code reads settings from
JSON script tags with a 'js-hypothesis-config' class name but the sidebar app
reads settings from JSON script tags with a 'js-hypothesis-settings'
class name. The boot script ended up using 'js-hypothesis-settings'
which is the default class name specified in `settings.js`.

We should use the same name everywhere, which needs to be
'js-hypothesis-config' since that is used by the public documentation
[1].

This commit changes the default class to `js-hypothesis-config` and
removes the ability to specify alternative class names when calling
`settings()`.

The extension and service will need to be updated to use the new class
name.

[1] See docs/config.md
parent e743717c
......@@ -18,7 +18,7 @@ function config(window_) {
// Parse config from `<script class="js-hypothesis-config">` tags
try {
Object.assign(options, settings(window_.document, 'js-hypothesis-config'));
Object.assign(options, settings(window_.document));
} catch (err) {
console.warn('Could not parse settings from js-hypothesis-config tags',
err);
......
......@@ -15,20 +15,16 @@ function assign(dest, src) {
* Return application configuration information from the host page.
*
* Exposes shared application settings, read from script tags with the
* class `settingsClass` which contain JSON content.
* class `js-hypothesis-config` which contain JSON content.
*
* If there are multiple such tags, the configuration from each is merged.
*
* @param {Document|Element} document - The root element to search for
* <script> settings tags.
* @param {string} settingsClass - The class name to match on <script> tags.
*/
function settings(document, settingsClass) {
if (!settingsClass) {
settingsClass = 'js-hypothesis-settings';
}
function settings(document) {
var settingsElements =
document.querySelectorAll('script.' + settingsClass);
document.querySelectorAll('script.js-hypothesis-config');
var config = {};
for (var i=0; i < settingsElements.length; i++) {
......
......@@ -2,11 +2,11 @@
var settings = require('../settings');
function createJSONScriptTag(obj, className) {
function createConfigElement(obj) {
var el = document.createElement('script');
el.type = 'application/json';
el.textContent = JSON.stringify(obj);
el.classList.add(className);
el.classList.add('js-hypothesis-config');
el.classList.add('js-settings-test');
return el;
}
......@@ -21,22 +21,14 @@ function removeJSONScriptTags() {
describe('settings', function () {
afterEach(removeJSONScriptTags);
it('reads config from .js-hypothesis-settings <script> tags', function () {
document.body.appendChild(createJSONScriptTag({key:'value'},
'js-hypothesis-settings'));
it('reads config from .js-hypothesis-config <script> tags', function () {
document.body.appendChild(createConfigElement({key:'value'}));
assert.deepEqual(settings(document), {key:'value'});
});
it('reads config from <script> tags with the specified class name', function () {
document.body.appendChild(createJSONScriptTag({foo:'bar'},
'js-custom-settings'));
assert.deepEqual(settings(document), {});
assert.deepEqual(settings(document, 'js-custom-settings'), {foo:'bar'});
});
it('merges settings from all config <script> tags', function () {
document.body.appendChild(createJSONScriptTag({a: 1}, 'settings'));
document.body.appendChild(createJSONScriptTag({b: 2}, 'settings'));
assert.deepEqual(settings(document, 'settings'), {a: 1, b: 2});
document.body.appendChild(createConfigElement({a: 1}));
document.body.appendChild(createConfigElement({b: 2}));
assert.deepEqual(settings(document), {a: 1, b: 2});
});
});
......@@ -5,7 +5,9 @@ require('../shared/polyfills');
var raven;
// Read settings rendered into sidebar app HTML by service/extension.
var settings = require('../shared/settings')(document);
if (settings.raven) {
// Initialize Raven. This is required at the top of this file
// so that it happens early in the app's startup flow
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment