Commit 20cd514a authored by Robert Knight's avatar Robert Knight

Fix `appBackgroundColor` theme property handling

Fix an incorrect call to `applyTheme` which passed an invalid theme
property name. This is a regression from when this component was converted
from AngularJS to Preact in 7bbb8a60.

Also improve the type definitions of `applyTheme` and add a test to help catch
such mistakes in future.

Fixes #2439
parent 1acba235
...@@ -15,6 +15,7 @@ import SvgIcon from '../../shared/components/svg-icon'; ...@@ -15,6 +15,7 @@ import SvgIcon from '../../shared/components/svg-icon';
/** /**
* @typedef {import('../../types/api').Annotation} Annotation * @typedef {import('../../types/api').Annotation} Annotation
* @typedef {import('../../types/config').MergedConfig} MergedConfig * @typedef {import('../../types/config').MergedConfig} MergedConfig
* @typedef {import('../util/theme').ThemeProperty} ThemeProperty
*/ */
/** /**
...@@ -56,6 +57,8 @@ function AnnotationPublishControl({ ...@@ -56,6 +57,8 @@ function AnnotationPublishControl({
const isPrivate = draft ? draft.isPrivate : !isShared(annotation.permissions); const isPrivate = draft ? draft.isPrivate : !isShared(annotation.permissions);
const publishDestination = isPrivate ? 'Only Me' : group.name; const publishDestination = isPrivate ? 'Only Me' : group.name;
/** @type {ThemeProperty[]} */
const themeProps = ['ctaTextColor', 'ctaBackgroundColor']; const themeProps = ['ctaTextColor', 'ctaBackgroundColor'];
// Revert changes to this annotation // Revert changes to this annotation
......
...@@ -77,7 +77,7 @@ function HypothesisApp({ ...@@ -77,7 +77,7 @@ function HypothesisApp({
}, [hasFetchedProfile, profile]); }, [hasFetchedProfile, profile]);
const backgroundStyle = useMemo( const backgroundStyle = useMemo(
() => applyTheme(['backgroundColor'], settings), () => applyTheme(['appBackgroundColor'], settings),
[settings] [settings]
); );
......
...@@ -7,6 +7,7 @@ import mockImportedComponents from '../../../test-util/mock-imported-components' ...@@ -7,6 +7,7 @@ import mockImportedComponents from '../../../test-util/mock-imported-components'
import HypothesisApp, { $imports } from '../hypothesis-app'; import HypothesisApp, { $imports } from '../hypothesis-app';
describe('HypothesisApp', () => { describe('HypothesisApp', () => {
let fakeApplyTheme;
let fakeUserAgent = null; let fakeUserAgent = null;
let fakeStore = null; let fakeStore = null;
let fakeAuth = null; let fakeAuth = null;
...@@ -33,6 +34,7 @@ describe('HypothesisApp', () => { ...@@ -33,6 +34,7 @@ describe('HypothesisApp', () => {
}; };
beforeEach(() => { beforeEach(() => {
fakeApplyTheme = sinon.stub().returns({});
fakeServiceConfig = sinon.stub(); fakeServiceConfig = sinon.stub();
fakeShouldAutoDisplayTutorial = sinon.stub().returns(false); fakeShouldAutoDisplayTutorial = sinon.stub().returns(false);
...@@ -89,6 +91,7 @@ describe('HypothesisApp', () => { ...@@ -89,6 +91,7 @@ describe('HypothesisApp', () => {
'../util/session': { '../util/session': {
shouldAutoDisplayTutorial: fakeShouldAutoDisplayTutorial, shouldAutoDisplayTutorial: fakeShouldAutoDisplayTutorial,
}, },
'../util/theme': { applyTheme: fakeApplyTheme },
}); });
}); });
...@@ -449,4 +452,15 @@ describe('HypothesisApp', () => { ...@@ -449,4 +452,15 @@ describe('HypothesisApp', () => {
}); });
}); });
}); });
it('applies theme config', () => {
const style = { backgroundColor: 'red' };
fakeApplyTheme.returns({ backgroundColor: 'red' });
const wrapper = createComponent();
const background = wrapper.find('.hypothesis-app');
assert.calledWith(fakeApplyTheme, ['appBackgroundColor'], fakeSettings);
assert.deepEqual(background.prop('style'), style);
});
}); });
/**
* @const {Object} All supported options for theming and their corresponding
* CSS property names (JS-style)
*/
const supportedThemeProperties = { const supportedThemeProperties = {
accentColor: 'color', accentColor: 'color',
appBackgroundColor: 'backgroundColor', appBackgroundColor: 'backgroundColor',
...@@ -11,6 +7,19 @@ const supportedThemeProperties = { ...@@ -11,6 +7,19 @@ const supportedThemeProperties = {
annotationFontFamily: 'fontFamily', annotationFontFamily: 'fontFamily',
}; };
/**
* Name of a theme element which can be configured.
*
* @typedef {keyof supportedThemeProperties} ThemeProperty
*/
/**
* Subset of the config from the host page which includes theme configuration.
*
* @typedef Settings
* @prop {Object.<ThemeProperty,string>} [branding]
*/
/** /**
* Return a React `style` object suitable for use as the value of the `style` * Return a React `style` object suitable for use as the value of the `style`
* attr in a React element, with styling rules for the requested set of * attr in a React element, with styling rules for the requested set of
...@@ -27,12 +36,11 @@ const supportedThemeProperties = { ...@@ -27,12 +36,11 @@ const supportedThemeProperties = {
* *
* See https://reactjs.org/docs/dom-elements.html#style * See https://reactjs.org/docs/dom-elements.html#style
* *
* @param {String[]} themeProperties Which of the supported theme properties * @param {ThemeProperty[]} themeProperties -
* should have applied rules in the `style` * Which of the supported theme properties should have applied rules in the `style`
* object * object
* @param {Object} settings A settings object, in which any `branding` * @param {Settings} settings
* property values are set * @return {Object.<string,string>} - Object that can be passed as the `style` prop
* @return {Object} An React-style style object
* *
* @example * @example
* let themeProperties = ['accentColor', 'ctaTextColor', 'foo']; * let themeProperties = ['accentColor', 'ctaTextColor', 'foo'];
...@@ -46,6 +54,7 @@ const supportedThemeProperties = { ...@@ -46,6 +54,7 @@ const supportedThemeProperties = {
* applyTheme(themeProperties, settings); // -> { color: '#ffc '} * applyTheme(themeProperties, settings); // -> { color: '#ffc '}
*/ */
export function applyTheme(themeProperties, settings) { export function applyTheme(themeProperties, settings) {
/** @type {Object.<string,string>} */
const style = {}; const style = {};
if (!settings.branding) { if (!settings.branding) {
return style; return style;
......
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