Merge pull request #1849 from hypothesis/remove-h-branding

Remove h-branding Angular directive

src/sidebar/components/hypothesis-app.js

@@ -5,6 +5,7 @@ import uiConstants from '../ui-constants';
 import { parseAccountID } from '../util/account-id';
 import isSidebar from '../util/is-sidebar';
 import { shouldAutoDisplayTutorial } from '../util/session';
+import { applyTheme } from '../util/theme';
 
 /**
  * Return the user's authentication status from their profile.
@@ -57,6 +58,8 @@ function HypothesisAppController(
   // used by templates to show an intermediate or loading state.
   this.auth = { status: 'unknown' };
 
+  this.backgroundStyle = applyTheme(['appBackgroundColor'], settings);
+
   // Check to see if we're in the sidebar, or on a standalone page such as
   // the stream page or an individual annotation page.
   this.isSidebar = isSidebar();

src/sidebar/directive/h-branding.js

-/**
- * The BrandingDirective brings theming configuration to our sidebar
- * by allowing the branding hypothesis settings to be reflected on items
- * that use this directive with the corresponding branding value.
- *
- * How to use:
- *   <element h-branding="supportedProp1, supportedProp2">
- *
- * Use `h-branding` to trigger this directive. Inside the attribute value,
- * add a comma separated list of what branding properties should be applied.
- * The attribute values match what the integrator would specify. For example,
- * if "superSpecialTextColor" is supported, the integrator could specify
- * `superSpecialTextColor: 'blue'` in the branding settings. Then any element that
- * included `h-branding="superSpecialTextColor"` would have blue placed on the
- * text's color.
- *
- * See below for the supported properties.
- */
-
-// @ngInject
-export default function BrandingDirective(settings) {
-  const _hasBranding = !!settings.branding;
-
-  // This is the list of supported property declarations
-  // we support. The key is the name and how it should be reflected in the
-  // settings by the integrator while the value (in the whitelist) is
-  // the type of .style property being set. The types are pretty simple for now
-  // and are a one-to-one mapping between the branding type and style property.
-  const _supportedPropSettings = {
-    accentColor: 'color',
-    appBackgroundColor: 'backgroundColor',
-    ctaBackgroundColor: 'backgroundColor',
-    ctaTextColor: 'color',
-    selectionFontFamily: 'fontFamily',
-    annotationFontFamily: 'fontFamily',
-  };
-
-  // filter all attribute values down to the supported
-  // branding properties
-  const _getValidBrandingAttribute = function(attrString) {
-    return attrString
-      .split(',')
-      .map(function(attr) {
-        return attr.trim();
-      })
-      .filter(function filterAgainstWhitelist(attr) {
-        return attr in _supportedPropSettings;
-      });
-  };
-
-  return {
-    restrict: 'A',
-    link: function(scope, $elem, attrs) {
-      if (_hasBranding) {
-        _getValidBrandingAttribute(attrs.hBranding).forEach(function(attr) {
-          const propVal = settings.branding[attr];
-          if (propVal) {
-            // the _supportedPropSettings holds the .style property name
-            // that is being set
-            $elem[0].style[_supportedPropSettings[attr]] = propVal;
-          }
-        });
-      }
-    },
-  };
-}

src/sidebar/directive/test/h-branding-fixtures.js

-export default [
-  // ALL SUPPORTED PROPERTIES
-  {
-    settings: { appBackgroundColor: 'blue' },
-    attrs: 'h-branding="appBackgroundColor"',
-    styleChanged: 'backgroundColor',
-    expectedPropValue: 'blue',
-  },
-  {
-    settings: { accentColor: 'red' },
-    attrs: 'h-branding="accentColor"',
-    styleChanged: 'color',
-    expectedPropValue: 'red',
-  },
-  {
-    settings: { ctaBackgroundColor: 'red' },
-    attrs: 'h-branding="ctaBackgroundColor"',
-    styleChanged: 'backgroundColor',
-    expectedPropValue: 'red',
-  },
-  {
-    settings: { ctaTextColor: 'yellow' },
-    attrs: 'h-branding="ctaTextColor"',
-    styleChanged: 'color',
-    expectedPropValue: 'yellow',
-  },
-  {
-    settings: { selectionFontFamily: 'georgia, arial' },
-    attrs: 'h-branding="selectionFontFamily"',
-    styleChanged: 'fontFamily',
-    expectedPropValue: 'georgia, arial',
-  },
-  {
-    settings: { annotationFontFamily: 'georgia, arial' },
-    attrs: 'h-branding="annotationFontFamily"',
-    styleChanged: 'fontFamily',
-    expectedPropValue: 'georgia, arial',
-  },
-
-  // EMPTY VALUE
-  {
-    settings: { appBackgroundColor: '' },
-    attrs: 'h-branding="appBackgroundColor"',
-    styleChanged: 'backgroundColor',
-    expectedPropValue: '',
-  },
-
-  // MULTIPLES
-  {
-    settings: { appBackgroundColor: 'blue', annotationFontFamily: 'arial' },
-    attrs: 'h-branding="appBackgroundColor, annotationFontFamily"',
-    styleChanged: ['backgroundColor', 'fontFamily'],
-    expectedPropValue: ['blue', 'arial'],
-  },
-  {
-    settings: {
-      appBackgroundColor: 'orange',
-      annotationFontFamily: 'helvetica',
-    },
-    attrs: 'h-branding="appBackgroundColor,annotationFontFamily"',
-    styleChanged: ['backgroundColor', 'fontFamily'],
-    expectedPropValue: ['orange', 'helvetica'],
-  },
-  {
-    settings: { appBackgroundColor: 'blue', annotationFontFamily: 'arial' },
-    attrs: 'h-branding="appBackgroundColor"',
-    styleChanged: ['backgroundColor', 'fontFamily'],
-    expectedPropValue: ['blue', ''],
-  },
-  {
-    settings: { appBackgroundColor: 'blue' },
-    attrs: 'h-branding="appBackgroundColor, annotationFontFamily"',
-    styleChanged: ['backgroundColor', 'fontFamily'],
-    expectedPropValue: ['blue', ''],
-  },
-];

src/sidebar/directive/test/h-branding-test.js

-import angular from 'angular';
-
-import hBranding from '../h-branding';
-
-import fixtures from './h-branding-fixtures';
-
-describe('BrandingDirective', function() {
-  let $compile;
-  let $rootScope;
-  let customSettings;
-
-  // Settings are set and frozen when the app initializes.
-  // This function allows us a way to quickly setup our environment
-  // with desired settings. Note, needs to be called for angular
-  // to be initialized for the test
-  const setSettingsAndBootApp = function() {
-    angular.module('app', []).directive('hBranding', hBranding);
-
-    angular.mock.module('app', {
-      settings: customSettings || {},
-    });
-
-    angular.mock.inject(function(_$compile_, _$rootScope_) {
-      $compile = _$compile_;
-      $rootScope = _$rootScope_;
-    });
-  };
-
-  // convenience method to only worry about what the test
-  // should be doing, applying settings variants.
-  const applyBrandingSettings = function(brandingSettings) {
-    customSettings = brandingSettings
-      ? {
-          branding: brandingSettings,
-        }
-      : null;
-
-    setSettingsAndBootApp();
-  };
-
-  // creates a new element with the attribute string provided.
-  // Allowing us to do many attribute configurations before
-  // compilation happens.
-  const makeElementWithAttrs = function(attrString) {
-    const $element = angular.element('<span ' + attrString + ' ></span>');
-    $compile($element)($rootScope.$new());
-    $rootScope.$digest();
-    return $element;
-  };
-
-  afterEach(function() {
-    customSettings = {};
-  });
-
-  it('branding should not happen on elements with unknown branding attributes', function() {
-    // note, we need to set some form of branding settings or this
-    // test will pass simply because branding isn't required
-    applyBrandingSettings({
-      appBackgroundColor: 'blue',
-    });
-
-    const el = makeElementWithAttrs('h-branding="randomBackgroundColor"');
-    assert.equal(el[0].style.backgroundColor, '');
-  });
-
-  fixtures.forEach(testCase => {
-    it('applies branding to elements', () => {
-      applyBrandingSettings(testCase.settings);
-
-      const el = makeElementWithAttrs(testCase.attrs);
-
-      if (Array.isArray(testCase.styleChanged)) {
-        // we expect that if styleChanged is an array
-        // that expectedPropValue will be an equal length array
-        testCase.styleChanged.forEach(function(styleChanged, index) {
-          assert.equal(
-            el[0].style[styleChanged],
-            testCase.expectedPropValue[index]
-          );
-        });
-      } else {
-        assert.equal(
-          el[0].style[testCase.styleChanged],
-          testCase.expectedPropValue
-        );
-      }
-    });
-  });
-});

src/sidebar/index.js

@@ -162,7 +162,6 @@ import threadList from './components/thread-list';
 
 // Angular directives.
 
-import hBrandingDirective from './directive/h-branding';
 import windowScrollDirective from './directive/window-scroll';
 
 // Services.
@@ -294,7 +293,6 @@ function startAngularApp(config) {
     .component('tagList', wrapComponent(TagList))
     .component('threadList', threadList)
     .component('topBar', wrapComponent(TopBar))
-    .directive('hBranding', hBrandingDirective)
     .directive('windowScroll', windowScrollDirective)
 
     // Register services, the store and utilities with Angular, so that

src/sidebar/templates/hypothesis-app.html

-<div class="app-content-wrapper js-thread-list-scroll-root" h-branding="appBackgroundColor">
+<div class="app-content-wrapper js-thread-list-scroll-root" ng-style="vm.backgroundStyle">
   <top-bar
     auth="vm.auth"
     on-login="vm.login()"